Skip to content

MLS Message types #2145

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

Merged
merged 8 commits into from
Mar 4, 2022
Merged

MLS Message types #2145

merged 8 commits into from
Mar 4, 2022

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Feb 21, 2022

This PR implements deserialisation for MLS message types

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti changed the base branch from develop to pcapriotti/mls-key-packages February 21, 2022 10:11
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-key-packages branch 3 times, most recently from 6ae2d14 to 631e529 Compare February 25, 2022 09:20
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-messages branch from 9e4c77d to 3e46af1 Compare February 25, 2022 12:55
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-key-packages branch from 8d240f8 to 39c8741 Compare February 28, 2022 10:41
Base automatically changed from pcapriotti/mls-key-packages to develop February 28, 2022 14:13
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-messages branch from b3fbf66 to cebb3a1 Compare February 28, 2022 15:00
@pcapriotti pcapriotti marked this pull request as ready for review March 1, 2022 13:00
@pcapriotti pcapriotti requested a review from mdimjasevic March 1, 2022 13:08
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 great!

Many of the data types you introduce in this PR have fields of Word* types. Would it make sense to wrap them with newtypes? If so, I would find such an alternative easier to read: once parsed, I don't care how many bytes something was in its serialised form, and on the other hand a solid type name might suggest what it is for, and it makes it harder to confuse values of the same Word* type.

@pcapriotti
Copy link
Contributor Author

This looks great!

Many of the data types you introduce in this PR have fields of Word* types. Would it make sense to wrap them with newtypes? If so, I would find such an alternative easier to read: once parsed, I don't care how many bytes something was in its serialised form, and on the other hand a solid type name might suggest what it is for, and it makes it harder to confuse values of the same Word* type.

I think it would just add a lot of noise with not that many benefits, in this case.

@pcapriotti pcapriotti merged commit f02329b into develop Mar 4, 2022
@pcapriotti pcapriotti deleted the pcapriotti/mls-messages branch March 4, 2022 06:40
@zebot zebot mentioned this pull request Mar 7, 2022
fisx pushed a commit that referenced this pull request Mar 7, 2022
* MLS message deserialisation

* Add simple message unit test

* Add application message unit test

* Implement welcome message deserialisation
@zebot zebot mentioned this pull request Mar 9, 2022
pcapriotti added a commit that referenced this pull request Mar 10, 2022
* MLS message deserialisation

* Add simple message unit test

* Add application message unit test

* Implement welcome message deserialisation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants