search

wireapp / wire-server

🇪🇺 Wire back-end services
https://wire.com
GNU Affero General Public License v3.0
2.62k stars 324 forks source link

integration: Allow MLS State to track multiple conversations #4329

Closed akshaymankar closed 2 weeks ago

akshaymankar commented 3 weeks ago

Checklist

pcapriotti commented 3 weeks ago

I haven't looked at this in detail, but I have a few questions.

What is the motivation for having multiple conversations? Conversations are mostly independent from one another, so any test that contains two conversations can always be written in a way where the two conversations exist one after the other.

Also, there seem to be multiple changes here that are not related to allowing multiple conversations. Why are we switching to a specific type for conversation parameters, instead of keeping with the existing conversion based on JSON values? Why is the ciphersuite being passed explicitly instead of using the one in the state?

akshaymankar commented 3 weeks ago

What is the motivation for having multiple conversations? Conversations are mostly independent from one another, so any test that contains two conversations can always be written in a way where the two conversations exist one after the other.

In performance tests we'd like to setup many users and then create different conversations with the same set of users. Having this assumption of one conversation and explicit state management was getting a bit too much, so we decided to do this.

Why are we switching to a specific type for conversation parameters, instead of keeping with the existing conversion based on JSON values?

The JSON values could be either a subconversation, a conversation or a domain-id pair in object form, this was getting a bit too confusing to be used as the key for the map in the state, so we decided to create the ConvId type.

Why is the ciphersuite being passed explicitly instead of using the one in the state?

We figured it was less confusing than to call modifyMLSState in the tests, sometimes some Util functions were doing this and keeping all that in mind while trying to make sense of the assertions was getting a bit confusing.