Skip to content
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

Add config type for joining from an external commit #1614

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

keks
Copy link
Contributor

@keks keks commented Jul 15, 2024

This PR adds a new sort of join config to make joining using external commits easier. I believe it's mostly pretty obvious, so just a few things:

  • The types have a generic lifetime parameter, because aad has been a reference in the past and I wanted to avoid needless copying.
  • I am not quite sure about which of the arguments belong into the config and which should be arguments. The ones I included are more on the "small data" side, whereas things like group info and ratchet tree are more on the "large protocol objects" side. I wasn't sure what to do with the credential_with_key. Feedback on this is very welcome.

Fixes #1607

@keks keks requested a review from a team as a code owner July 15, 2024 15:12
@keks keks requested a review from kkohbrok July 15, 2024 15:12
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 72.56637% with 31 lines in your changes missing coverage. Please review.

Project coverage is 84.85%. Comparing base (5fb5e62) to head (ce247cf).

Files Patch % Lines
openmls/src/group/mls_group/config.rs 65.55% 31 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1614      +/-   ##
==========================================
- Coverage   84.92%   84.85%   -0.08%     
==========================================
  Files         187      187              
  Lines       21692    21786      +94     
==========================================
+ Hits        18423    18486      +63     
- Misses       3269     3300      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raphaelrobert
Copy link
Member

Thanks for tackling it! I think AAD really is the outlier here and I should get working on #1521 instead. I thought about it some more, and the current way the AAD is handled only came into existence because I didn't have any better ideas at the time, not because it's actually how folks are using it. All discussions I've had so far point to wanting an ad-hoc solution for the AAD. And since it's a breaking change for the storage provider, it would be good to get this in for v0.6. I'll get started with it.

@keks
Copy link
Contributor Author

keks commented Jul 16, 2024

Just a note: this PR also removes the --message-format json from the cargo rustdoc check in CI so it's more readable.

@raphaelrobert raphaelrobert self-assigned this Jul 16, 2024
@keks keks requested review from raphaelrobert and removed request for kkohbrok July 16, 2024 08:35
@keks
Copy link
Contributor Author

keks commented Jul 16, 2024

Do you want to do your aad changes inside this PR or do you want to do it separately @raphaelrobert? Not sure how to interpret your self-assign :)

@raphaelrobert
Copy link
Member

Do you want to do your aad changes inside this PR or do you want to do it separately @raphaelrobert? Not sure how to interpret your self-assign :)

Self-assign is always for reviews. The PR is already is here already: #1615, but I'm still adding tests. I guess it would make sense to get in #1615 first because then you can drop the lifetime.

@franziskuskiefer
Copy link
Contributor

What's the plan here? @keks did you want to close this?

@keks
Copy link
Contributor Author

keks commented Aug 19, 2024

On a call we discussed restructuring this such that the external commit join config contains the regular join config, instead of mirroring all the fields. With that, and the changes to the AAD, I think it would make most sense to just start over on top of master, if we want to get this in.

Should I have another stab at this or should we close this unfinished?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Better group creation flow for External Commits
3 participants