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

Import DID support for daf-react-native-libsodium #257

Conversation

devrajsinghrawat
Copy link
Contributor

#228

Feature branch to support import Identity for daf-react-native-libsodium package

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 initiative to contribute.
I'll mark this as a work in progress. Please signal when it's ready for review.

We are getting closer to releasing version 7 which has some significant changes that may lead to conflicts. For this reason, it might be better if you base your changes off of the beta branch. That way, they will be easier to incorporate into the codebase.

@@ -98,4 +98,9 @@ export class KeyManagementSystem extends AbstractKeyManagementSystem {
debug('Deleting', kid)
return await this.keyStore.delete(kid)
}

async importKey(serializedKey: SerializedKey) {
await this.keyStore.set(serializedKey.kid, serializedKey)
Copy link
Member

Choose a reason for hiding this comment

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

There should be some checks regarding the key capabilities.
Either here, or maybe at the keyStore level.
For example, if a SerializedKey is provided without a privateKeyHex component then it will be unusable after import.
Depending on the keyStore or the type of key other checks may be needed.

Copy link
Contributor Author

@devrajsinghrawat devrajsinghrawat Nov 10, 2020

Choose a reason for hiding this comment

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

Hi @mirceanis ,

This fix is for daf-react-native-libsodium version 6 where the corresponding daf-libsodium version 6 has the import function implemented in the exact fashion without any checks.

A code changes corresponds to version 7 can have a different PR.

Do you still think that those check should be added ? Pls suggest
Thanks

Copy link
Member

@mirceanis mirceanis Nov 10, 2020

Choose a reason for hiding this comment

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

ok, so this is not work in progress actually. It makes sense as a fix to daf v6.

I will be merging a bunch of PRs and it will be included in a next hotfix release for daf.

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.

2 participants