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!: native DIDDocuments and proper DID resolution interface #799

Merged
merged 80 commits into from
Oct 26, 2023

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Sep 29, 2023

Second attempt to fix https://github.com/KILTprotocol/ticket/issues/2804. It also fixes https://github.com/KILTprotocol/ticket/issues/2340.

Relevant changes

DidDetails.ts

  • Types that are only used within the DID package have been moved here from the @kiltprotocol/types package
  • Most utility functions have been removed. We could re-add some functions or add new utility functions that apply both to light and full DIDs here if we need/want to.

FullDidDetails.ts

  • Renaming of verificationKey to verificationMethod

LightDidDetails.ts

  • Types that are only used to deal with light DIDs have been moved inside here
  • Renaming of verificationKey to verificationMethod where needed

DidResolver.ts

  • Implements the new (and proper) DID resolution standard as defined in DID Core and DID Resolution. The interface for this is declared in @kiltprotocol/types, in the DidResolverV2.ts module. I tried to carry over as much as possible from the current implementation of the resolver.
  • Unit tests still to write.

Did.chain.ts, Did.rpc.ts, Did.signature.ts

  • They have been updated to refer to the new DidDocument type, dealing as much as possible with verification methods and DID Documents instead of our current intermediate representation.

Where the "raw" keypair type still appears

To aid usability, I left the DidVerificationKey and DidEncryptionKey types in a few places. For instance, when creating a light DID, so that the key does not have to first be converted into a fully-fledged verification method, for which the controller URI is still not known. Also the Did2.chain.ts includes mentions of DidKeys in the getStoreTxFromInput, whereas the other function, getStoreTxFromDocument takes a DID Document and composes a DID creation tx out of it. The old getStoreTx is hence split into these two functions.

What else

This is WIP, and I have mostly added new files in the DID and types packages so that old stuff is not broken yet. Comments, optimizations, etc will be improved once we agree this is the right level of abstraction. Additional utility functions can always be added if needed, as long as we don't break this newly-created semantic where everything is a DID Document, a verification method, or a service.

@ntn-x2 ntn-x2 self-assigned this Sep 29, 2023
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

submitting a first batch of comment to not leave them hanging any longer. more to follow!

packages/did/src/Did.chain.ts Outdated Show resolved Hide resolved
packages/types/src/Did.ts Show resolved Hide resolved
packages/did/src/Did.chain.ts Outdated Show resolved Hide resolved
packages/did/src/Did.rpc.ts Outdated Show resolved Hide resolved
packages/types/src/Did.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Looks really damn nice, great job and thanks for the contribution! I'm thinking that at some point we may want to introduce two resolution options: one that controls which key type is returned (to aid backwards-compatibility; if anybody is already working with actual did documents at this point) and one that controls whether you get fully qualified DID URIs vs fragments as your ids. But I'd probably just add that after this merge so you can move on.

Just get rid of the Buffer though, we shouldn't need to use that.

packages/did/src/Did.signature.ts Show resolved Hide resolved
packages/did/src/Did.utils.ts Outdated Show resolved Hide resolved
packages/did/src/Did.utils.ts Show resolved Hide resolved
packages/did/src/DidDetails/index.ts Outdated Show resolved Hide resolved
packages/utils/src/Crypto.spec.ts Outdated Show resolved Hide resolved
packages/types/src/DidResolver.ts Outdated Show resolved Hide resolved
@ntn-x2 ntn-x2 force-pushed the aa/native-did-attempt-2 branch from 77c0db4 to caf9e92 Compare October 20, 2023 10:35
@ntn-x2
Copy link
Member Author

ntn-x2 commented Oct 20, 2023

@rflechtner the reason for the failing unit tests is a bit more complicated to solve. The reason is not only the missing context, but also the fact that the verification suite expects the verification method in the legacy format, while now it's returned in a Multikey format.
I fixed the lack of context here, and also re-uploaded the new context to IPFS and updated the CID (you can verify it via the Crust resolver, which is the only one I have seen working fine). As for fixing the tests, we need to decide whether we want to fix them in this PR by adding the query param to the resolver so that the old key format is returned, or if we want to merge this PR as is and fix those in a different PR. At most, I could add some tests for the new DataIntegrity suite which expects verification methods in Multikey formats, but I feel that's rather part of #801. What do you think?

@rflechtner
Copy link
Contributor

@rflechtner the reason for the failing unit tests is a bit more complicated to solve. The reason is not only the missing context, but also the fact that the verification suite expects the verification method in the legacy format, while now it's returned in a Multikey format.

I fixed the lack of context here, and also re-uploaded the new context to IPFS and updated the CID (you can verify it via the Crust resolver, which is the only one I have seen working fine). As for fixing the tests, we need to decide whether we want to fix them in this PR by adding the query param to the resolver so that the old key format is returned, or if we want to merge this PR as is and fix those in a different PR. At most, I could add some tests for the new DataIntegrity suite which expects verification methods in Multikey formats, but I feel that's rather part of #801. What do you think?

Isn't that fixed best by amending the suite so it accepts both multikeys and the legacy key representations? I think this would be fair in general, given that we have been using the legacy format for so long. It is also something that i would want to see more generally in the sdk, but thought we may re-add support for this later, after you merged. For now I'd implement minimal support for legacy keys just for the broken suites, just enough to get the currently failing tests to pass

@ntn-x2 ntn-x2 marked this pull request as ready for review October 23, 2023 12:05
@ntn-x2 ntn-x2 requested a review from rflechtner October 23, 2023 15:00
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Amazing! Just two little things and then we're done here :)

packages/did/src/Did.chain.ts Outdated Show resolved Hide resolved
packages/did/src/Did.utils.ts Outdated Show resolved Hide resolved
packages/did/src/Did.utils.ts Outdated Show resolved Hide resolved
packages/did/src/Did.utils.ts Show resolved Hide resolved
packages/vc-export/src/documentLoader.ts Outdated Show resolved Hide resolved
packages/did/src/Did.signature.ts Show resolved Hide resolved
packages/types/src/Did.ts Outdated Show resolved Hide resolved
packages/types/src/Did.ts Show resolved Hide resolved
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

🚀

@ntn-x2 ntn-x2 merged commit 801211d into vc-refactor Oct 26, 2023
10 of 13 checks passed
@ntn-x2 ntn-x2 deleted the aa/native-did-attempt-2 branch October 26, 2023 07:56
@rflechtner rflechtner restored the aa/native-did-attempt-2 branch October 26, 2023 08:23
rflechtner added a commit that referenced this pull request Oct 26, 2023
#799)"

This reverts squash commit 801211d to merge this PR with a merge commit instead.
@rflechtner rflechtner deleted the aa/native-did-attempt-2 branch July 24, 2024 14:54
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.

3 participants