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: new high-level credentialing interfaces #811

Merged
merged 25 commits into from
Dec 5, 2023

Conversation

rflechtner
Copy link
Contributor

@rflechtner rflechtner commented Nov 29, 2023

fixes KILTProtocol/ticket#2964

Introduces high-level credentialing interfaces (e.g., issue / createPresentation / verifyPresentation) which are agnostic, on the surface, of which type of credential or proof is being used and provide a seamless experience even when we introduce support for additional proof types.

Some changes to some previously implemented functionality were made as part of this:

  • Presentation verification, except for verification of presentation proofs, has been moved to the new verifier interface implementations, as it relies on other verifier implementations (i.e., verifyCredential/checkStatus).
  • Presentation signing has been moved into a more general DataIntegrity.signWithDid function; Presentation creation is available as a separate step. A function that combines creation and signing is available in the new holder interfaces.
  • Block timestamps are now represented as Date instances throughout.
  • Transaction authorisation and submission handlers used in KiltAttestationProofV1.issue have been refactored.
  • Credentials-related functionality in core has been moved to core/credentials
  • As a new feature, implemented the possibility to sign transactions using the cryptosuites-style signers.

Not covered here:

  • Finalising what and how functions are available from sdk-js, which is meant to be the long-term stable api
  • Helpers for producing the DID / identity interfaces
  • Switching to using signers in tests where possible

How to test:

Tests are pending, but all interfaces are being covered in bundle tests.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)
    • Either PR or Ticket to update the Docs
    • Link the PR/Ticket here

@rflechtner rflechtner marked this pull request as ready for review November 29, 2023 12:28
Copy link
Member

@Dudleyneedham Dudleyneedham left a comment

Choose a reason for hiding this comment

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

Comments here throughout:

  • We are moving to issuer as language. This needs to be aligned with the comms team. lets make sure that this happens.
  • A few questions to be answered
  • The code itself is good, but its hard to know how it will be in production without using it. It will be interesting in the future when we can start implementing it.
  • it feels as if we have gone back to the roles model.

packages/core/src/credentials/V1/KiltAttestationProofV1.ts Outdated Show resolved Hide resolved
packages/core/src/credentials/V1/KiltAttestationProofV1.ts Outdated Show resolved Hide resolved
packages/core/src/credentials/V1/KiltAttestationProofV1.ts Outdated Show resolved Hide resolved
packages/core/src/credentials/verifier.ts Show resolved Hide resolved
packages/core/src/credentials/verifier.ts Outdated Show resolved Hide resolved
tests/bundle/bundle-test.ts Show resolved Hide resolved
tests/integration/utils.ts Show resolved Hide resolved
@rflechtner
Copy link
Contributor Author

  • We are moving to issuer as language. This needs to be aligned with the comms team. lets make sure that this happens.

True, I chose issuer over attester as this is the language used by the VC specs.

  • it feels as if we have gone back to the roles model.

This is mostly for organising the code; but I also export the functions under those actors namespaces right now because I think it helps a lot for understanding what you can do with a credential. But this can be discussed; we can also export it all in one namespace if that's better

const { byDid, byAlgorithm } = Signers.select

/**
* Signs a document with a DID-related signer.
Copy link
Contributor

@ChrisChinchilla ChrisChinchilla Dec 5, 2023

Choose a reason for hiding this comment

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

Suggested change
* Signs a document with a DID-related signer.
* Signs and updates a document with a new DID-related signer.

Maybe…?

@Dudleyneedham
Copy link
Member

  • We are moving to issuer as language. This needs to be aligned with the comms team. lets make sure that this happens.

True, I chose issuer over attester as this is the language used by the VC specs.

  • it feels as if we have gone back to the roles model.

This is mostly for organising the code; but I also export the functions under those actors namespaces right now because I think it helps a lot for understanding what you can do with a credential. But this can be discussed; we can also export it all in one namespace if that's better

This should also be noted to comms that we are doing this. I think we were going to collect a bunch of new languages and roll out the change in the SDK version 1. It would be good to identify this so we can offer this for the company to communicate to the community.

Regarding the changes it does help with reading and organising the code. I prefer the actors API vibe. It feels more suited towards our communications of how to use the technology.

@rflechtner
Copy link
Contributor Author

This should also be noted to comms that we are doing this. I think we were going to collect a bunch of new languages and roll out the change in the SDK version 1. It would be good to identify this so we can offer this for the company to communicate to the community.

I think jour fixe would be a good place to bring this up.

Copy link
Member

@Dudleyneedham Dudleyneedham left a comment

Choose a reason for hiding this comment

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

LGTM

@rflechtner rflechtner merged commit fdba28f into vc-refactor Dec 5, 2023
12 of 15 checks passed
@rflechtner rflechtner deleted the rf-credential-interface branch December 5, 2023 15:37
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