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: legacy credentials package #786

Merged
merged 8 commits into from
Oct 4, 2023

Conversation

rflechtner
Copy link
Contributor

@rflechtner rflechtner commented Aug 9, 2023

addresses https://github.com/KILTprotocol/ticket/issues/2156

Adds a package for legacy kilt credential support, which also becomes the new home for transformations between VCs and legacy credentials.

How to test:

Tests included. Doesn't hurt to try exporting a credential with it though.

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 changed the title Rf legacy credentials package feat: legacy credentials package Aug 9, 2023
@rflechtner rflechtner force-pushed the rf-legacy-credentials-package branch from 252968e to b07f2c8 Compare August 11, 2023 14:44
@rflechtner rflechtner force-pushed the rf-legacy-credentials-package branch from 3176d80 to 0a3fbf0 Compare August 23, 2023 19:11
@rflechtner rflechtner requested a review from ntn-x2 August 23, 2023 21:31
@rflechtner rflechtner self-assigned this Aug 23, 2023
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

I assume the Claim.ts and Credential.ts have simply been copy-pasted from the core package? I also did not check the tests in detail yet, also not sure I would be able to provide much feedback on those.

In general, I like a lot the API of the legacy package, with one question about we can make creating a VC from a legacy credential easier for people.

packages/vc-export/src/__mocks__/testData.ts Outdated Show resolved Hide resolved
packages/vc-export/src/__mocks__/testData.ts Show resolved Hide resolved
packages/legacy-credentials/src/vcInterop.ts Outdated Show resolved Hide resolved
packages/legacy-credentials/src/vcInterop.ts Outdated Show resolved Hide resolved
packages/legacy-credentials/src/vcInterop.ts Show resolved Hide resolved
packages/legacy-credentials/src/vcInterop.spec.ts Outdated Show resolved Hide resolved
packages/legacy-credentials/src/utils.ts Outdated Show resolved Hide resolved
packages/legacy-credentials/src/utils.ts Show resolved Hide resolved
@rflechtner
Copy link
Contributor Author

I assume the Claim.ts and Credential.ts have simply been copy-pasted from the core package?

Yes indeed, with only minuscule adjustments.

In general, I like a lot the API of the legacy package, with one question about we can make creating a VC from a legacy credential eas_ier_ for people.

This is what is not super easy per se, because it involves an indexer service that we have yet to implement - once we have that we may try to hide this aspect a little better?

Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

LGTM so far!

@rflechtner rflechtner changed the base branch from develop to vc-refactor October 4, 2023 10:50
@rflechtner rflechtner marked this pull request as ready for review October 4, 2023 12:23
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

Not much has changed, and nothing seems to be off.

@rflechtner rflechtner merged commit af692dc into vc-refactor Oct 4, 2023
15 checks passed
@rflechtner rflechtner deleted the rf-legacy-credentials-package branch October 4, 2023 12:45
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.

2 participants