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.

Sign up for GitHub

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

Jump to bottom

Update xmtp/openmls to latest crate HEAD #558

Closed
wants to merge 10 commits into from
Closed

Update xmtp/openmls to latest crate HEAD #558

wants to merge 10 commits into from

Conversation

zombieobject
Copy link
Contributor

No description provided.

@zombieobject zombieobject self-assigned this Mar 20, 2024
@zombieobject zombieobject requested a review from a team March 20, 2024 00:35
@neekolas
Copy link
Contributor

I would expect there to be updates to the root Cargo.toml pointing to different rev values in this PR

@zombieobject
Copy link
Contributor Author

zombieobject commented Mar 20, 2024 •
edited
Loading

Reference in ne

I would expect there to be updates to the root Cargo.toml pointing to different rev values in this PR

Really good point! I wasn't paying attention to the path in my terminal. I missed the fact that cargo is smart enough to stick to the module. Thank you! I will update accordingly.

@zombieobject zombieobject marked this pull request as draft March 20, 2024 19:29
@zombieobject
Copy link
Contributor Author

Putting this PR into "Draft" mode while I address compiler errors from the update. 🛠️

neekolas
neekolas reviewed Mar 21, 2024
View reviewed changes
xmtp_mls/src/hpke.rs Outdated
@@ -49,7 +49,7 @@ pub fn decrypt_welcome(
.read::<HpkePrivateKey>(hpke_public_key)
.ok_or(HpkeError::KeyNotFound)?;

let ciphertext = HpkeCiphertext::tls_deserialize_exact(ciphertext)?;
let ciphertext = HpkeCiphertext::tls_deserialize_exact(ciphertext).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

We're trying to keep this codebase unwrap free. If the ? doesn't work anymore it's probably because you need a new error variant that supports the error type from tls_deserialize_exact

@neekolas
Copy link
Contributor

Looking at this commit all the credential.identity() errors make sense. They changed the API so that now you have to convert the credential into a BasicCredential using into() or try_into() and then you'll get access to the identity method.

neekolas
neekolas reviewed Mar 21, 2024
View reviewed changes
xmtp_mls/src/identity.rs Outdated Show resolved Hide resolved
@zombieobject
Copy link
Contributor Author

Looking at this commit all the credential.identity() errors make sense. They changed the API so that now you have to convert the credential into a BasicCredential using into() or try_into() and then you'll get access to the identity method.

Thanks for the guidance @neekolas! I started down that path initially, but was concerned at how much change I was introducing. With this knowledge, I'll proceed with confidence that it's the right direction. 🙂

@zombieobject
Copy link
Contributor Author

@nplasterer @neekolas Now that I fixed the Identity calls, I also have a stashed commit ready to go that addresses the TLS Serialization family of call sites to remove the use of .unwrap().

As I began digging into those, I realized there was some work to unify the error messaging passed from module(s) to the various call sites in order to cover both TLS and Credential work.

Now that I see how this all works together, I intend to bundle that all together tomorrow in a new push and then start on the inbound MLS Group related errors afterwards. 👨🏼‍🚒

@zombieobject
Copy link
Contributor Author

@nplasterer @neekolas I have pushed the changes to standardize the error types and surface the error messages passed from TLS serialization/deserialization. Next I'm on to figuring out the new Groups API and the changes/updates needed to migrate to this new API.

neekolas
neekolas reviewed Mar 22, 2024
View reviewed changes
xmtp_mls/src/client.rs Outdated
@@ -106,9 +108,9 @@ pub enum MessageProcessingError {
#[error("storage error: {0}")]
Storage(#[from] crate::storage::StorageError),
#[error("tls deserialization: {0}")]
TlsDeserialization(#[from] tls_codec::Error),
TlsDeserialization(#[from] openmls::prelude::Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is now a more general MLS error type, maybe we need to rename our mapped type. I'm assuming openmls::prelude::Error now includes more things than TlsDeserialization errors, although correct me if I'm wrong.

Copy link
Contributor Author

@zombieobject zombieobject Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! I added this early on before better understanding the error processing logic. I have updated the mapping here as well as consolidated the use openmls:: declarations.

neekolas
neekolas reviewed Mar 22, 2024
View reviewed changes
xmtp_mls/src/groups/members.rs Outdated
@@ -37,8 +41,9 @@ pub fn aggregate_member_list(openmls_group: &OpenMlsGroup) -> Result<Vec<GroupMe
let member_map: HashMap<String, GroupMember> = openmls_group
.members()
.filter_map(|member| {
let basic_credential = BasicCredential::try_from(&member.credential).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

One more unwrap here

neekolas
neekolas reviewed Mar 22, 2024
View reviewed changes
xmtp_mls/src/identity.rs Outdated
self.set_credential(credential)?;
}

// Register the installation with the server
let kp = self.new_key_package(provider)?;
let kp_bytes = kp.tls_serialize_detached()?;
let kp_bytes = kp.tls_serialize_detached()
.map_err(|e| IdentityError::TlsSerialization(e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above for the #[from]. You should be able to automatically convert these

@zombieobject
Copy link
Contributor Author

New groups API integration is complete and all build errors/warnings have been resolved! ✨

Tomorrow I intend to complete following before converting this to PR:

  • Rebase off of main and resolve existing file conflicts
  • Address @neekolas feedback on Error types and remaining unwrap()
  • Verify that all tests still pass 😅

@zombieobject zombieobject force-pushed the em/openmls-update branch 5 times, most recently from 65991ed to 7ad5246 Compare March 26, 2024 18:08
@zombieobject
Copy link
Contributor Author

Creating a new PR for the final submission. This new branch will contain some merge conflict and additional assistance from @tuddman. 🫶🏼

@zombieobject zombieobject deleted the em/openmls-update branch March 26, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neekolas neekolas neekolas left review comments

Assignees

@zombieobject zombieobject

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zombieobject @neekolas