-
Notifications
You must be signed in to change notification settings - Fork 28
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: DID package refactor #447
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dudleyneedham
approved these changes
Jan 24, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tjwelde
reviewed
Jan 26, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like it. Just some questions and comments because of consistent coding style
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DID package refactor
This PR fixes https://github.com/KILTprotocol/ticket/issues/1756 and fixes https://github.com/KILTprotocol/ticket/issues/1712 by making DIDs and everything that uses DIDs (e.g., extrinsic DID signing) easier to use. The spec doc can be found in this HackMD document. Most of the things match the implementation, others have deviated a bit during the development.
What's changed
Most of the design was specified in the design document above, with the main different compared to the current codebase being that there are two components involved for DIDs:
DidDetails
abstract class which is then derive byLightDidDetails
andFullDidDetails
. These classes are meant to be used on the owner side (e.g., for decrypting/signing stuff, or to sign extrinsics).IDidDetails
interface which is implemented by bothLightDidDetails
andFullDidDetails
. This interface is meant to be used on the receiver side, hence it is exposed by the resolver and by methods that take arguments referring to the "other communicating party" (e.g., during credential exchange). This interface exposes basic methods that one would expect from a DID, i.e., retrieving key(s), and service endpoints by ID or by type.Other changes
Some other important changes are the following:
DefaultResolver
toDidResolver
What's missing
Batched DID operations (for creations and updates)
This PR has not implemented a DID builder, which would make the generation of full DID way easier. The initial design can be found in the document linked above, while a almost complete (untested) implementation can be found in this PR history, specifically commit 1cdc6e is the one where the whole code for builders was removed since it was decided to put it in a different PR.
Updated getting-started and examples guides
Another thing that is left out of this PR is the documentations. The reason is that most of the docs will have to change again fairly shortly when batched operations are added, hence I though that it would not be the best use of time to change all docs now and then change them again in few weeks. We won't have an official release that external people will be using anyway before we add the batching, so I don't think it's a big problem if integration tests for the docs are failing for a while. There's anyway a ticket tracking that.