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

First draft of did-pkh method. #1052

Merged
merged 33 commits into from
Nov 30, 2022

Conversation

dchagastelles
Copy link
Contributor

@dchagastelles dchagastelles commented Nov 2, 2022

  • Create new pkh-did-provider
    The pkh-did-provider supports only eip155 network id (did:pkh:eip155:...) so for now the network id is hardcoded, but we could easily parameterize to support other networks.

  • Add resolve did-pkh tests
    Tests passing

  • Add create did-pkh identifier test
    Tests passing

  • Add VC creation/verification using did-pkh identifier
    Tests passing

Detail

Currently not using pkh-did-resolver package (error import). Code momentarily copied to did-provider-pkh resolver.

What issue is this PR fixing

Resolves #1024

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.

- Create new pkh-did-provider
- Add resolve did-pkh tests
- Add create did-pkh identidfier test
- Add VC creation/verification using did-pkh identifier

Currently not using pkh-did-resolver package (error import).
Code momentarily copied to did-provider-pkh resolver.
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 contributing this and apologies for taking so long to review.

I made some suggestions to fix the failing test.
The DID string for eip155 is formed using the ethereum address, not the compressed public key. This was also the cause of the test failures you were seeing.

Also, CAIP10/eip155 require chainIDs to be used in the blockchainAccountId, not network names.
eip155:1:0xaddress instead of eip155:mainnet:0xaddress

packages/did-provider-pkh/CHANGELOG.md Outdated Show resolved Hide resolved
packages/did-provider-ion/dist/tsdoc-metadata.json Outdated Show resolved Hide resolved
packages/did-provider-pkh/package.json Outdated Show resolved Hide resolved
packages/did-provider-pkh/package.json Outdated Show resolved Hide resolved
packages/did-provider-pkh/package.json Outdated Show resolved Hide resolved
packages/did-provider-pkh/src/pkh-did-provider.ts Outdated Show resolved Hide resolved
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.

I added another couple of suggested fixes to the tests.
Please take a look.

__tests__/shared/verifiableDataLD.ts Outdated Show resolved Hide resolved
__tests__/shared/verifiableDataLD.ts Outdated Show resolved Hide resolved
dchagastelles and others added 13 commits November 21, 2022 13:51
-Use chainId only and not network name
-clean up params
-fix tests
- Create new pkh-did-provider
- Add resolve did-pkh tests
- Add create did-pkh identidfier test
- Add VC creation/verification using did-pkh identifier

Currently not using pkh-did-resolver package (error import).
Code momentarily copied to did-provider-pkh resolver.
-Use chainId only and not network name
-clean up params
-fix tests
Co-authored-by: Mircea Nistor <mirceanis@gmail.com>
Co-authored-by: Mircea Nistor <mirceanis@gmail.com>
Co-authored-by: Mircea Nistor <mirceanis@gmail.com>
@dchagastelles dchagastelles marked this pull request as ready for review November 21, 2022 20:36
@dchagastelles dchagastelles changed the title WIP: First draft of did-pkh method. First draft of did-pkh method. Nov 21, 2022
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #1052 (32336b2) into next (125bf42) will decrease coverage by 0.48%.
The diff coverage is 77.26%.

❗ Current head 32336b2 differs from pull request most recent head f64690c. Consider uploading reports for the commit f64690c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1052      +/-   ##
==========================================
- Coverage   80.25%   79.76%   -0.49%     
==========================================
  Files         118      130      +12     
  Lines        4056     4699     +643     
  Branches      875     1011     +136     
==========================================
+ Hits         3255     3748     +493     
- Misses        798      951     +153     
+ Partials        3        0       -3     

@nickreynolds
Copy link
Contributor

now that tests are passing, it would probably be a good time to rebase this branch against next to get rid of the extraneous commits

dchagastelles and others added 8 commits November 23, 2022 11:06
- Create new pkh-did-provider
- Add resolve did-pkh tests
- Add create did-pkh identidfier test
- Add VC creation/verification using did-pkh identifier

Currently not using pkh-did-resolver package (error import).
Code momentarily copied to did-provider-pkh resolver.
-Use chainId only and not network name
-clean up params
-fix tests
- Create new pkh-did-provider
- Add resolve did-pkh tests
- Add create did-pkh identidfier test
- Add VC creation/verification using did-pkh identifier

Currently not using pkh-did-resolver package (error import).
Code momentarily copied to did-provider-pkh resolver.
-Use chainId only and not network name
-clean up params
-fix tests
Co-authored-by: Mircea Nistor <mirceanis@gmail.com>
Co-authored-by: Mircea Nistor <mirceanis@gmail.com>
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.

Looks good.
Thanks for contributing this!

package.json Outdated Show resolved Hide resolved
@mirceanis mirceanis merged commit 5ad0bfb into decentralized-identity:next Nov 30, 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.

[proposal] add did:pkh support
3 participants