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

IdentityUpdate serialization #633

Merged
merged 6 commits into from
Apr 11, 2024
Merged

IdentityUpdate serialization #633

merged 6 commits into from
Apr 11, 2024

Conversation

neekolas
Copy link
Contributor

@neekolas neekolas commented Apr 7, 2024 •
edited
Loading

tl;dr

  • Adds serialization and deserialization for IdentityUpdate
  • Adds a missing inbox_id field to a few of the action types
  • Scaffolds out the Signature types. Still need real verification of signatures and recovery addresses.
  • Rename the IdentityUpdateBuilder to SignatureRequestBuilder
  • Make current with final version of protos (moved the inbox_id to the IdentityUpdate instead of being on each action)

This was referenced Apr 7, 2024
Merged
Merged
Merged
@neekolas Graphite App
Copy link
Contributor Author

neekolas commented Apr 7, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @neekolas and the rest of your teammates on Graphite Graphite

@neekolas neekolas mentioned this pull request Apr 7, 2024
Merged
@neekolas neekolas marked this pull request as ready for review April 7, 2024 22:37
@neekolas neekolas requested a review from a team as a code owner April 7, 2024 22:37
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch 2 times, most recently from b09be7f to 22dc869 Compare April 7, 2024 22:39
@neekolas neekolas force-pushed the 04-05-add_identity_update_builder branch from c2b729b to a93bfe6 Compare April 7, 2024 22:42
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch from 22dc869 to da73388 Compare April 7, 2024 22:42
@neekolas neekolas force-pushed the 04-05-add_identity_update_builder branch from a93bfe6 to b4e75e4 Compare April 7, 2024 22:42
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch from da73388 to 4744e6f Compare April 7, 2024 22:42
neekolas
neekolas commented Apr 7, 2024
View reviewed changes
dev/gen_protos.sh Outdated Show resolved Hide resolved
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch 2 times, most recently from 23adb18 to 23db819 Compare April 7, 2024 23:12
@neekolas neekolas force-pushed the 04-05-add_identity_update_builder branch from b4e75e4 to 9c2569e Compare April 8, 2024 16:10
Base automatically changed from 04-05-add_identity_update_builder to main April 8, 2024 22:57
@37ng 37ng mentioned this pull request Apr 10, 2024
Closed
@insipx
Copy link
Contributor

insipx commented Apr 10, 2024 •
edited
Loading

This overall looks good to me, there's just some clippy lints to resolve and merging with main required (minus feedback from last automerged pr)

@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch 3 times, most recently from c700fd0 to 4b2d5dc Compare April 10, 2024 17:15
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch 4 times, most recently from 6102158 to 574af6f Compare April 10, 2024 17:54
@neekolas neekolas requested a review from insipx April 10, 2024 19:10
richardhuaaa
richardhuaaa reviewed Apr 10, 2024
View reviewed changes
xmtp_id/src/associations/signature.rs
Comment on lines +213 to +216
// TODO: Two steps needed here:
// 1. Verify the RecoverableEcdsaSignature and make sure it recovers to the public key specified in the `signed_public_key`
// 2. Verify the wallet signature on the `signed_public_key`
// Return the wallet address
Copy link
Contributor

Choose a reason for hiding this comment

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

In case it's helpful, for your next PR - there should be an implementation of this already that you can copy/paste/refactor, same with the todo's above

Copy link
Contributor

Choose a reason for hiding this comment

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

ethers crate supports this and I'm working on this issue.

btw, if anyone knows how to convert bytes(Vec, [u8] etc.) to/from different signature types(ethers, k256 etc.), that would be so helpful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest writing a TryFrom<...> implementation for our types, and if their libraries already have TryFrom implemented you can use that internally. That way you don't have to worry about constraints around implementing traits for foreign types.

xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
xmtp_id/src/associations/unsigned_actions.rs Outdated Show resolved Hide resolved
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch from 574af6f to 3576910 Compare April 10, 2024 21:05
@neekolas
Merge branch '04-07-identityupdate_serialization' of github.com:xmtp/…
b6bfc4d 
…libxmtp into 04-07-identityupdate_serialization
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch from 3576910 to b6bfc4d Compare April 10, 2024 21:12
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch from 421bf0c to dc8215a Compare April 10, 2024 21:44
@neekolas
Merge branch '04-07-identityupdate_serialization' of github.com:xmtp/…
3678ad4 
…libxmtp into 04-07-identityupdate_serialization
@neekolas neekolas force-pushed the 04-07-identityupdate_serialization branch from dc8215a to 3678ad4 Compare April 10, 2024 21:51
@neekolas
Copy link
Contributor Author

@richardhuaaa I've updated to match the spacing in your comments and added hardcoded text in the tests

@neekolas neekolas mentioned this pull request Apr 10, 2024
Merged
richardhuaaa
richardhuaaa approved these changes Apr 11, 2024
View reviewed changes
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Thanks for handling my nits! I preferred the original time format you had to ISO8601, but that's fine

@neekolas neekolas merged commit 121a867 into main Apr 11, 2024
11 checks passed
@neekolas neekolas deleted the 04-07-identityupdate_serialization branch April 11, 2024 00:47
@neekolas Graphite App
Copy link
Contributor Author

Merge activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardhuaaa richardhuaaa richardhuaaa approved these changes

@insipx insipx Awaiting requested review from insipx

@37ng 37ng Awaiting requested review from 37ng

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@neekolas @insipx @richardhuaaa @37ng