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

feat: ION DID Provider implementation #987

Merged

Conversation

nklomp
Copy link
Member

@nklomp nklomp commented Aug 10, 2022

What issue is this PR fixing

closes #336 and closes #440

What is being changed

Implement ION DID Provider. Seeking initial feedback before rebasing to Veramo and opening a PR there.

Changes:

  • Do not depend on ion-tools
  • Depend on external ION POW SDK.
  • Update and recovery keys, including key rotations
  • Full implementation of all Identifier methods, with the exception of the recent updateIdentifier method

Known issues:

  • The ION PoW lib on which this PR depends works on Node. It is not tested yet on React-native, but we do have a React-Native version of the dependency. I will ensure it will work on both, since we need it @Sphereon anyway
  • Adding/Updating services/keys does not get propagated on chain. Needs more investigation. Microsoft gives back a 200 response for them, but it never ends up in the DID Document.

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran yarn, yarn build, yarn test, yarn test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because _________, and I am aware that a PR without tests will likely get rejected.

Details

If applicable, add screen captures, error messages or stack traces to help explain your problem.

@nklomp nklomp mentioned this pull request Aug 10, 2022
@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #987 (a41b321) into next (125bf42) will increase coverage by 0.65%.
The diff coverage is 83.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #987      +/-   ##
==========================================
+ Coverage   80.25%   80.90%   +0.65%     
==========================================
  Files         118      124       +6     
  Lines        4056     4451     +395     
  Branches      875      950      +75     
==========================================
+ Hits         3255     3601     +346     
- Misses        798      847      +49     
  Partials        3        3              

nklomp added 3 commits August 23, 2022 19:12
…ve using update ION-POW dep, that depends on a new isomorphic

-argon2 implementation
…ion_did

# Conflicts:
#	packages/did-provider-ion/README.md
#	packages/did-provider-ion/package.json
#	packages/did-provider-ion/src/ion-did-provider.ts
@nklomp
Copy link
Member Author

nklomp commented Aug 23, 2022

Updated our ION POW package to now depend on a new isomorphic Argon2 hash package created by us. Verified to be working in browser, node and react-native. For react-native a peer dependency is put in the package.json. This dep needs to be installed, since native components are needed for Argon2 in RN. Except for installing the peer dep, which is mentioned in the Readme, a user does not have to change anything else on RN.

Still looking into the update issue. As far as we can tell right now this only is happening on the public Microsoft hosted ION node. I will get in contact with my contacts within the Microsoft team to see if this is a known issue or not. In the mean time we are setting up our own ION node, but that takes a bit of time, given the components involved.

@rkreutzer
Copy link
Contributor

Can we flag the update issue as technical debt and merge the PR? It's holding up any app from being able to support MS Entra.

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

There is a tiny nit-pick about the import paths in one of the tests, but otherwise I wish all PRs were this beautiful :)

packages/did-provider-ion/__tests__/functions.test.ts Outdated Show resolved Hide resolved
packages/did-provider-ion/src/functions.ts Show resolved Hide resolved
@nklomp
Copy link
Member Author

nklomp commented Oct 4, 2022

Thank you for the contribution!

There is a tiny nit-pick about the import paths in one of the tests, but otherwise I wish all PRs were this beautiful :)

Thanks. Updated the PR, and created tickets for the TD: #1019 and suggestion: #1021

@nklomp
Copy link
Member Author

nklomp commented Oct 4, 2022

Updated Veramo dep versions as it gave an error on the latest CI run

@mirceanis
Copy link
Member

Updated Veramo dep versions as it gave an error on the latest CI run

Thanks, I was trying to do that on my end but I didn't know if I could push to your PR branch.
If this builds fine, we'll be merging it soon

@mirceanis mirceanis merged commit 594571c into decentralized-identity:next Oct 10, 2022
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.

Add support for did:ion
3 participants