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

feature/SDK-24 #225

Merged
merged 25 commits into from
Jul 25, 2024
Merged

feature/SDK-24 #225

merged 25 commits into from
Jul 25, 2024

Conversation

sanderPostma
Copy link
Contributor

@sanderPostma sanderPostma commented Jul 15, 2024

No description provided.

@sanderPostma sanderPostma marked this pull request as ready for review July 15, 2024 20:55
@sanderPostma sanderPostma marked this pull request as draft July 15, 2024 20:56
Copy link
Contributor

@nklomp nklomp left a comment

Choose a reason for hiding this comment

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

Some initial remarks

packages/credential-store/src/agent/CredentialStore.ts Outdated Show resolved Hide resolved
* @param withFilter - Optional additional filter criteria.
* @returns A FindDigitalCredentialArgs array for filtering verifiable credentials by role.
*/
export const verifiableCredentialForRoleFilter = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only available for VCs? We need this for C, VP, P as well, as all of these can have roles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a helper to avoid duplicate code and make it easier to fetch only VC's
If you need one of the other document types, don't use "verifiableCredentialForRoleFilter"
Create your own filter or create a verifiablePresentationForRoleFilter function when having duplicate code.

packages/oid4vci-holder/src/agent/OID4VCIHolder.ts Outdated Show resolved Hide resolved
const digitalCredentials = await this.crsGetUniqueCredentials({
filter: [
{
documentType: DocumentType.VC, // TODO does crmGetCredentialsByClaims need to support VPs as well?
Copy link
Contributor

Choose a reason for hiding this comment

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

At least C should be supported, but a ticket should be created to indeed also support (V)P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

# Conflicts:
#	packages/ebsi-support/src/agent/EbsiSupport.ts
#	packages/siopv2-oid4vp-op-auth/src/session/OID4VP.ts
@sanderPostma sanderPostma marked this pull request as ready for review July 16, 2024 13:23
@@ -634,7 +634,7 @@ export class OID4VCIHolder implements IAgentPlugin {
})

const credential = {
id: issuanceOpt.credentialConfigurationId ?? issuanceOpt.id,
id: (issuanceOpt.credentialConfigurationId ?? issuanceOpt.id) as string | undefined, // cast due to error: Type 'unknown' is not assignable to type 'string | undefined'.
Copy link
Contributor

@nklomp nklomp Jul 25, 2024

Choose a reason for hiding this comment

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

Suggested change
id: (issuanceOpt.credentialConfigurationId ?? issuanceOpt.id) as string | undefined, // cast due to error: Type 'unknown' is not assignable to type 'string | undefined'.
id: (issuanceOpt.credentialConfigurationId ?? issuanceOpt.id as string | undefined), // cast due to error: Type 'unknown' is not assignable to type 'string | undefined'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pulled in something very recent from develop where id is determined a few lines up, so let's see if that builds

Copy link
Contributor

Choose a reason for hiding this comment

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

Well the bigger question is why it fails. I know that the lower level libs have some changes. But that has been fixed in develop. The fact develop builds and this branch doesn't suggest either the PR is reverting something, not using the latest develop, or new code is introduced where this happens. Please investigate first before continuing @sanderPostma

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, now I understand what you mean. Yes this was fixed in lates develop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nklomp
Looks like build are passing now indeed

@sanderPostma sanderPostma merged commit 150eb38 into develop Jul 25, 2024
2 checks passed
@nklomp nklomp deleted the feature/SDK-24 branch July 25, 2024 19:52
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