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(sdk): Improved did:key create function #598

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

DRK3
Copy link
Collaborator

@DRK3 DRK3 commented Sep 11, 2023

Added a new function for creating did:key DIDs. This new standalone function directly takes in a key instead of a keyWriter or keyReader, giving the caller more flexibility in terms of key management. It also minimizes input parameters to only what's required, helping reduce confusion caused by unused parameters.

@cla-bot cla-bot bot added the cla-signed label Sep 11, 2023
@DRK3 DRK3 force-pushed the ImprovedDIDKeyCreateFunction branch from b13c34d to 47f9383 Compare September 11, 2023 23:40
Comment on lines 34 to 38
// Workaround: when the did:key VDR creates a DID for ed25519, Ed25519VerificationKey2018 is the expected
// verification method.
publicKeyBytes, err := jwk.JWK.PublicKeyBytes()
if err != nil {
return nil, wrapper.ToMobileError(err)
Copy link
Member

Choose a reason for hiding this comment

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

Should these be in go pkg rather than mobile binding pkg ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was also wondering the same thing. I wasn't sure if we wanted this behaviour to be common to all bindings that use the Go SDK or if it should be specific to the binding implementation (gomobile, WASM). Since you were wondering the same thing, I think perhaps it makes more sense for it to be common, so I'll update the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@DRK3 DRK3 force-pushed the ImprovedDIDKeyCreateFunction branch from 47f9383 to 3589737 Compare September 12, 2023 15:10
Added a new function for creating did:key DIDs. This new standalone function directly takes in a key instead of a keyWriter or keyReader, giving the caller more flexibility in terms of key management. It also minimizes input parameters to only what's required, helping reduce confusion caused by unused parameters.

Signed-off-by: Derek Trider <[email protected]>
@DRK3 DRK3 force-pushed the ImprovedDIDKeyCreateFunction branch from 3589737 to c3e7434 Compare September 12, 2023 15:13
@sonarcloud
Copy link

sonarcloud bot commented Sep 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
3.9% 3.9% Duplication

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 92.85% and project coverage change: -0.10% ⚠️

Comparison is base (32bc40e) 89.83% compared to head (c3e7434) 89.74%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
- Coverage   89.83%   89.74%   -0.10%     
==========================================
  Files          98      100       +2     
  Lines        3927     3960      +33     
==========================================
+ Hits         3528     3554      +26     
- Misses        265      271       +6     
- Partials      134      135       +1     
Files Changed Coverage Δ
cmd/wallet-sdk-gomobile/didkey/didkey.go 81.81% <81.81%> (ø)
pkg/did/creator/key/key.go 77.27% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@birtony birtony merged commit 2ffa912 into trustbloc:main Sep 12, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants