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

Update ODIS SDK to accept any type of identifier #9985

Merged
merged 36 commits into from
Dec 8, 2022

Conversation

isabellewei
Copy link
Contributor

@isabellewei isabellewei commented Oct 28, 2022

Description

Creates new functions to interact with ODIS that are not phone number specific

Instead of using the phone number specific identifier and hashing logic that was in the base and utils libraries, I created new functions for the equivalent directly in the odis sdk. I figured that no one would really use that logic outside of the odis sdk anyways (is that a fair assumption?) Eventually I'd like to remove all phone number specific ODIS code

I wanted to use DID methods for the prefixes, but seems like as of now there's only one for twitter, but not email or phone number yet. (As an aside, would be cool if we drafted DID methods for them ourselves!)

Tested

tests updated
backward compatibility test to ensure phone number identifiers are the same

Related issues

Backwards compatibility

  • some functions in phone-number-identifier.ts are kept to maintain backwards compatibility

Documentation

The set of community facing docs that have been added/modified because of this change

@isabellewei isabellewei requested a review from a team October 28, 2022 00:03
@isabellewei isabellewei requested a review from a team as a code owner October 28, 2022 00:03
@isabellewei isabellewei marked this pull request as draft October 28, 2022 00:03
@isabellewei isabellewei force-pushed the isabellewei/allIdentifiers branch 2 times, most recently from 554509f to 4f802df Compare October 28, 2022 00:49
@isabellewei isabellewei force-pushed the isabellewei/allIdentifiers branch from 4f802df to e9f1eb8 Compare October 28, 2022 00:56
@isabellewei isabellewei marked this pull request as ready for review October 31, 2022 14:10
@eelanagaraj
Copy link
Contributor

more generally, in addition to the unit tests we should ideally sanity-check this by manually testing that we get the same pepper/identifier for a phone number with the current version of the SDK & updated version from this PR. Reason being is that a breaking change (i.e. we are missing a '+' sign somewhere or added a space, or whatever) would be really hard to spot IMO and would be very difficult to debug!

packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/phone-number-identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/phone-number-identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.test.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alecps alecps left a comment

Choose a reason for hiding this comment

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

The two main things I see that need addressing before we can merge this are the same as Eela pointed out.

  1. We need a backwards compatibility check somewhere to ensure that we're getting the same identifiers as before. This could maybe be added to the combiner e2e tests
  2. It might be better if different prefixes yielded different peppers. If we decide to keep things as they are, we should be sure to document our reasoning.

Though it would be amazing to squeeze this feature into this round of releases, there's a part of me that feels a bit uneasy about rushing this and adding new complexity right at the finish line. Normally this type of feature would be given much more time for careful design / review, and I wonder if it might be best to adjust our expectations to getting this merged after we've released ASv2. I'm not saying we can't get it in beforehand, but we should step back and weigh both options

@isabellewei isabellewei requested a review from a team as a code owner November 18, 2022 21:51
@isabellewei isabellewei requested a review from alvarof2 November 18, 2022 21:51
@isabellewei
Copy link
Contributor Author

After chatting with @nategraf, agreed that it's best to also append prefixes before blinding the identifiers, but skip this step for phone numbers to maintain backwards compatibility.

I've also added a backwards compatibility test for phone numbers that import an old version of the @celo/identity sdk to compare against the new changes

Copy link
Contributor

@alecps alecps left a comment

Choose a reason for hiding this comment

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

looking good so far!

packages/sdk/identity/src/odis/phone-number-identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/phone-number-identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
@isabellewei isabellewei requested a review from nategraf November 23, 2022 18:43
@isabellewei isabellewei requested a review from a team November 29, 2022 02:04
Copy link
Contributor

@nategraf nategraf left a comment

Choose a reason for hiding this comment

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

Made a few comments, one with relation to the hashing of identifiers to suggest a two step hash. Overall it looks good.

packages/attestation-service/package.json Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Show resolved Hide resolved
packages/sdk/identity/src/odis/identifier.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alecps alecps left a comment

Choose a reason for hiding this comment

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

Nice work!

@alecps alecps enabled auto-merge (squash) December 7, 2022 22:54
@alecps alecps merged commit 4041432 into master Dec 8, 2022
@mcortesi mcortesi deleted the isabellewei/allIdentifiers branch August 23, 2023 12:54
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.

Update SDK to allow for all types of identifiers
5 participants