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(vc tests): use the schema validator package #2502

Merged
merged 6 commits into from
Feb 23, 2024

Conversation

jonalvarezz
Copy link
Contributor

Context

P-142 : Enable use of JSONSchema in VC introduced stricter JSON Schemas for the generated VC. The schemas can be found at vc-jsonschema repo.

This change, uses the schema validator package distributed from the same repo above to assert the generated VCs' schemas.

Challenge

What's another good place we could add this schema check in our codebase? Ideally, VC's template should from the schema, but that's out of the scope of this task.

I found that this schema check faces the same main blocker of "we can't generate all VCs in development due to the lack of right accounts/ data provider setup"

Copy link

linear bot commented Feb 19, 2024

@@ -38,7 +37,7 @@ export async function assertFailedEvent(
type EventLike = Parameters<typeof isFailed>[0];
const ievents: EventLike[] = events.map(({ event }) => event);
const failedEvent = ievents.filter(isFailed);
/*
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe beyond the reach of this PR, but this way of event assertion is a bit awful - would something like context.api.events.identityManagement.LinkIdentityFailed.is(event) and then checking event.detail work?

cc @Verin1005

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it'd be great, but first we need to fix typescript to pick the right types. Same issue as: https://linear.app/litentry/issue/P-508/avoid-as-unknown-as-when-possible

Otherwise, the type guard on .is(event) won't work.

It took us some time to get that right on IDH 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

We're using is(event) approach in di evm tests. So I feel like as unknown as might be being overused. I'll double-check when I start this task.

Copy link
Contributor

@0xverin 0xverin 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 good; I like this lib. >3

@@ -166,85 +166,3 @@ export const unconfiguredAssertions = [
},
},
];
export const jsonSchema = {
Copy link
Contributor

@0xverin 0xverin Feb 22, 2024

Choose a reason for hiding this comment

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

I tested it locally, and it works. In fact, this crude jsonSchema should have been removed and replaced long ago. It was created during the validation of the first VC request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Verin1005 , if there isn't anything against, can I get the approval check to merge it? 😄

@jonalvarezz jonalvarezz enabled auto-merge (squash) February 22, 2024 16:02
@jonalvarezz jonalvarezz merged commit 159570a into dev Feb 23, 2024
31 checks passed
@0xverin 0xverin deleted the p-416-ensure-that-assertions-follow-the-json-schemas branch February 23, 2024 07:20
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.

None yet

3 participants