-
Notifications
You must be signed in to change notification settings - Fork 83
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
Check for last resort in core_group/new_from_welcome #1503
Check for last resort in core_group/new_from_welcome #1503
Conversation
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.
Thanks for the PR! It looks like it solves the issue. To confirm that it does and to ensure we don't run into this sort of thing in the future, would you be so kind as to add a simple test? You can add the test to mls_group/test_mls_group.rs
, where you can find inspiration on how to set up the test as well. I think it's better to have the test at the MlsGroup
(rather than the CoreGroup
) level, since we're likely to move all provider access to MlsGroup
level eventually anyway (see #1207).
(Also you'll have to run cargo fmt
on the code before we can merge.)
See 050d330 for the test. Open to feedback/guidance on robustness of the test procedure for this case. |
See 4253a26 for the |
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.
Thanks for adding the test and making the requested changes! This looks good to me, but clippy has a few complaints. Once you sort those out we can get this merged.
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.
Sorry, one last thing: Could you add an entry to the changelog indicating that this bug was fixed? You can find it in CHANGELOG.md.
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.
Excellent, thanks!
Closes #1502
This PR just copies the logic in group::MlsGroup::new_from_welcome(...), which checks for last resort extension in keypackage, to group::CoreGroup::new_from_welcome(...).