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 legacy credential type #425

Merged
merged 28 commits into from
Feb 2, 2024
Merged

Add legacy credential type #425

merged 28 commits into from
Feb 2, 2024

Conversation

richardhuaaa
Copy link
Contributor

@richardhuaaa richardhuaaa commented Jan 18, 2024

This adds the legacy credential type. Support for generating these credentials will be added in the next PR.

  • Apply the changes from Add legacy v2-signed keys proto#131
  • Add ValidatedLegacySignedPublicKey type and tests
  • Add LegacyCreateIdentityAssociation - cannot add tests until I have support for serializing into proto, will add in next PR
  • Significant refactoring. We use separate classes in separate files for different associations. The strong typing ensures we can't mix them up (for example confusing grant messaging access with revoke messaging access).

Because the protos have changed, some steps are required to avoid errors for those who already have libxmtp running:

  • Run ./dev/down && ./dev/up from libxmtp. This rebuilds the validation service, and clears out any data stored on the server.
  • Remove any existing databases, for example from the CLI and Android/iOS/flutter/RN

@@ -6,7 +6,7 @@ if ! cargo install --list | grep "protoc-gen-prost-crate" > /dev/null; then
fi
fi

if ! buf generate https://github.com/xmtp/proto.git#branch=main,subdir=proto; then
if ! buf generate https://github.com/xmtp/proto.git#branch=rich/legacy-keys-2,subdir=proto; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will switch back to main before merging

@richardhuaaa richardhuaaa changed the title Add support for legacy credentials Add legacy credential type Jan 19, 2024
@richardhuaaa richardhuaaa marked this pull request as ready for review January 19, 2024 03:57
@richardhuaaa richardhuaaa requested a review from a team January 19, 2024 03:57
@richardhuaaa richardhuaaa marked this pull request as draft February 1, 2024 19:36
@richardhuaaa richardhuaaa changed the base branch from main to benchmark February 2, 2024 00:40
@richardhuaaa richardhuaaa changed the base branch from benchmark to main February 2, 2024 00:40
@richardhuaaa richardhuaaa changed the base branch from main to rich/signing-fix February 2, 2024 00:42
@richardhuaaa richardhuaaa changed the base branch from rich/signing-fix to main February 2, 2024 00:42
@richardhuaaa richardhuaaa marked this pull request as ready for review February 2, 2024 00:51
@@ -48,6 +48,7 @@ toml = "0.7.4"
tracing = "0.1.37"
xmtp_cryptography = { path = "../xmtp_cryptography" }
xmtp_proto = { path = "../xmtp_proto", features = ["proto_full"] }
xmtp_v2 = { path = "../xmtp_v2" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think once we get to shaving fat off the WASM build we're going to want to extract the functions we need from this crate to avoid bloat. Or get really aggressive with features.

Problem for another day

pub(crate) account_address: Address,
pub(crate) installation_public_key: Vec<u8>,
pub(crate) created_ns: u64,
iso8601_time: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to store the iso8601 time on the struct. Seems like we can just generate it as-needed.

Copy link
Contributor Author

@richardhuaaa richardhuaaa Feb 2, 2024

Choose a reason for hiding this comment

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

The act of getting the time from the created_ns can produce an error, and it is better to validate the incoming data when creating the struct and allow callers to handle the error then and there, rather than when it is eventually used to produce text()

use xmtp_proto::xmtp::message_contents::SignedPublicKey as LegacySignedPublicKeyProto;

#[tokio::test]
async fn validate_good_key() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the hardcoded text fixtures. These are going to be helpful for catching regressions

Copy link
Contributor

@neekolas neekolas left a comment

Choose a reason for hiding this comment

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

Think this is looking good

richardhuaaa added a commit to xmtp/proto that referenced this pull request Feb 2, 2024
- Add LegacyCreateIdentityAssociation
- Split out GrantMessagingAccessAssociation and RevokeMessagingAccessAssociation from Eip191Association - not all EIP191 associations will have the same data, and this allows for stronger type-checking
- Change time format from ISO8601 string to nanoseconds since epoch - this creates consistency with the old SignedPublicKey type. The ISO8601 string can be constructed at the time of generating the signature text.

Will land xmtp/libxmtp#425 immediately after this PR lands
@richardhuaaa richardhuaaa enabled auto-merge (squash) February 2, 2024 23:26
@richardhuaaa richardhuaaa merged commit 6351982 into main Feb 2, 2024
10 checks passed
@richardhuaaa richardhuaaa deleted the rich/legacy-association branch February 2, 2024 23:29
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