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: public credentials and asset DIDs #677

Merged
merged 70 commits into from
Nov 10, 2022
Merged

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 3, 2022

Fixes https://github.com/KILTprotocol/ticket/issues/2092 and fixes https://github.com/KILTprotocol/ticket/issues/2175.

Original draft document (might not be entirely up to date with what's implemented here) -> https://hackmd.io/@EAX5fDsURyKJJ3GahxyZbA/Skoh424wq.

What is this

This PR adds support for the long-awaited public credentials and asset DIDs.

A public credential behaves differently than a traditional KILT credential since it is publicly accessible by anyone that has access to a full archive node.
A credential is not stored inside the blockchain state, but a pointer to the block number in which it was created is stored instead.
When retrieving a credential, two operations must be performed:

  1. retrieve the details from the blockchain state
  2. parse the block with the given number "hunting" for the right call, which contains the content of the credential (i.e., the actual claims)

How are credentials supposed to be used and shared [SEE PR DISCUSSION BELOW FOR NEWER INFORMATION]

I envisioned that public credentials would never be shared directly between parties, because they are public. They would be used in one of the following two ways:

  1. By asset: a party know the AssetDID of an asset and queries a KILT full node to obtain all the public credentials issued to it
  2. By identifier: two parties can exchange credentials by simply sharing their IDs, which is sufficient to retrieve the full content by combining the query and RPC calls as mentioned above. There might be a better way to share credentials, such that the credential is shared as a whole, but I did not find a good way to modify the functions to make that work, as basically performing all the required checks involves fetching the actual credential from the blockchain and verifying it, which becomes redundant if the credential has already been shared.

Most of the code has been inspired by how credentials and attestations have been modelled in the SDK, since a public credential is kind of a combination of both.

What about AssetDIDs

Since they are simply a wrapper over CAIP-19 identifier, which are simply "regexable", the new package does not do much. Eventually, we will develop an AssetDID resolver that can attach additional information to an AssetDID, such as the credentials linked to it, but I think that is out of scope from this PR, which represents already a good-enough PoC.

@arty-name
Copy link
Collaborator

I just recalled, there’s a new guy on the block: Array.prototype.findLast(). It’s too early to use it, though, but maybe in a year.

@ntn-x2
Copy link
Member Author

ntn-x2 commented Nov 9, 2022

@arty-name @rflechtner I consider this PR ready to be merged. There is something seemingly broken with CType verification, which will be tackled in a different PR. For now, the test for that check is skipped, and we can enable it again once we have a fix for the CTypes.
Please re-review the last changes, otherwise I would say we can proceed and merge, so that other people can start playing around with it. I won't be available to do any more major work after today, so I hope that anything left comes up today so I can address it.

I tested the verifyCredential in the integration tests rather than in the unit tests as I had issued mocking the api stuff. We need both createType and mockResult, and one works only on the mocked API while the other one only on the decorated API. Testing those in the integration tests (which also makes sense since there is a call to a runtime API) removes the need for mocking those, and I think it is also a reasonable choice.

packages/asset-did/src/AssetDid.ts Outdated Show resolved Hide resolved
packages/core/src/publicCredential/PublicCredential.ts Outdated Show resolved Hide resolved
packages/core/src/publicCredential/PublicCredential.ts Outdated Show resolved Hide resolved
packages/core/src/publicCredential/PublicCredential.ts Outdated Show resolved Hide resolved
@arty-name
Copy link
Collaborator

I approve the code and leave the higher level decisions to Raphael

@arty-name
Copy link
Collaborator

Looking at the updated error message in the latest commit, I started thinking that the ID of the public credential is its hash of a kind. If we use this more specific word "hash" (as we do for CType), we won’t have to explain to other developers what are the IDs of public credentials, and how we generate them, and what a mismatching ID means. Because many developers already know these facts about hashes.

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.

There's a suggestion or two but I think we're done here!

packages/core/src/claim/Claim.ts Outdated Show resolved Hide resolved
packages/testing/src/mocks/mockedApi.ts Outdated Show resolved Hide resolved
@ntn-x2 ntn-x2 merged commit 560bb55 into develop Nov 10, 2022
@ntn-x2 ntn-x2 deleted the aa/public-credentials branch November 10, 2022 15:39
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.

4 participants