Skip to content

integration: Allow MLS State to track multiple conversations#4329

Merged
battermann merged 14 commits intodevelopfrom
wpb-11416-mls-state-multi-conv
Nov 8, 2024
Merged

integration: Allow MLS State to track multiple conversations#4329
battermann merged 14 commits intodevelopfrom
wpb-11416-mls-state-multi-conv

Conversation

@akshaymankar
Copy link
Member

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d No changelog.
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 4, 2024
@pcapriotti
Copy link
Contributor

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
Copy link
Member Author

akshaymankar commented Nov 5, 2024

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.

@akshaymankar akshaymankar force-pushed the wpb-11416-mls-state-multi-conv branch from fe9b6ca to 684fa20 Compare November 5, 2024 15:32
@mdimjasevic mdimjasevic self-requested a review November 5, 2024 15:47
@akshaymankar akshaymankar force-pushed the wpb-11416-mls-state-multi-conv branch from e8e6512 to 9fa5582 Compare November 5, 2024 16:36
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! There are a few lines with old commented out code in the tests that should be removed.

@battermann battermann merged commit 30dbb07 into develop Nov 8, 2024
@battermann battermann deleted the wpb-11416-mls-state-multi-conv branch November 8, 2024 09:57
@b1pb1p b1pb1p added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

echoes: unplanned Any work item that isn’t part of the product or technical roadmap. ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants