Skip to content

feat: nested ctype val #919

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

Closed
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 105 additions & 5 deletions packages/credentials/src/V1/KiltCredentialV1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { hexToU8a } from '@polkadot/util'
import { base58Encode } from '@polkadot/util-crypto'
import * as Kilt from "@kiltprotocol/sdk-js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the credentials package is imported by the sdk-js package this creates a circular dependency and will not work. We also don't do blanket imports in the sdk implementation. Be specific, only import what you need. CType stuff is implemented in the credentials package, this should not be necessary.

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 at: 77b9c3d


import { JsonSchema, SDKErrors } from '@kiltprotocol/utils'
import type {
Expand Down Expand Up @@ -327,15 +328,76 @@ export function fromInput({

const cachingCTypeLoader = newCachingCTypeLoader()

// Helper function to check if CType is nested
function cTypeTypeFinder(cType: ICType): boolean {
function hasRef(obj: any): boolean {
if (typeof obj !== 'object' || obj === null) return false

if ('$ref' in obj) return true

return Object.values(obj).some(value => {
if (Array.isArray(value)) {
return value.some(item => hasRef(item))
}
if (typeof value === 'object') {
return hasRef(value)
}
return false
})
}
return hasRef(cType.properties)
}
Copy link
Member

Choose a reason for hiding this comment

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

No need to make another Named function inside this function.

Suggested change
// Helper function to check if CType is nested
function cTypeTypeFinder(cType: ICType): boolean {
function hasRef(obj: any): boolean {
if (typeof obj !== 'object' || obj === null) return false
if ('$ref' in obj) return true
return Object.values(obj).some(value => {
if (Array.isArray(value)) {
return value.some(item => hasRef(item))
}
if (typeof value === 'object') {
return hasRef(value)
}
return false
})
}
return hasRef(cType.properties)
}
// Helper function to check if CType is nested
function cTypeTypeFinder(cType: ICType): boolean {
if (typeof cType.properties !== 'object' || cType.properties === null) return false
if ('$ref' in cType.properties) return true
return Object.values(cType.properties).some(value => {
if (Array.isArray(value)) {
return value.some(item => hasRef(item))
}
if (typeof value === 'object') {
return hasRef(value)
}
return false
})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you're defining a function to call it recursively, but can't you just do that with the outer function directly? Also, more generally, I think all that you need is the second function; if you're already recursing through the CType, collect all the references, we will need them eventually.

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 at: 77b9c3d


// Helper function to extract unique references from CType
function extractUniqueReferences(cType: ICType): Set<string> {
Copy link
Member

Choose a reason for hiding this comment

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

As above, change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here I can somewhat see the point of having an inner function; alternatively, you could solve it by taking an optional second argument:

Suggested change
function extractUniqueReferences(cType: ICType): Set<string> {
function extractUniqueReferences(cType: ICType, references=new Set<string>()): Set<string> {

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 at: 77b9c3d

const references = new Set<string>()

function processValue(value: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use any, use unknown

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 at: 77b9c3d

if (typeof value !== 'object' || value === null) return

if ('$ref' in value) {
const ref = value['$ref']
// Extract KILT CType reference
if (ref.startsWith('kilt:ctype:')) {
// Get first part split by #/
const baseRef = ref.split('#/')[0]
references.add(baseRef)
}
}

// Check all values of the object recursively
Object.values(value).forEach(v => processValue(v))
}

processValue(cType.properties)
return references
}


/**
* Validates the claims in the VC's `credentialSubject` against a CType definition.
* Supports both nested and non-nested CType validation.
*
* For non-nested CTypes:
* - Validates claims directly against the CType schema
*
* For nested CTypes:
* - Automatically detects nested structure through $ref properties
* - Fetches referenced CTypes from the blockchain
* - Performs validation against the main CType and all referenced CTypes
*
* @param credential A {@link KiltCredentialV1} type verifiable credential.
* @param credential.credentialSubject The credentialSubject to be validated.
* @param credential.type The credential's types.
* @param options Options map.
* @param options.cTypes One or more CType definitions to be used for validation. If `loadCTypes` is set to `false`, validation will fail if the definition of the credential's CType is not given.
* @param options.loadCTypes A function to load CType definitions that are not in `cTypes`. Defaults to using the {@link newCachingCTypeLoader | CachingCTypeLoader}. If set to `false` or `undefined`, no additional CTypes will be loaded.
* @param options.cTypes One or more CType definitions to be used for validation. If loadCTypes is set to false, validation will fail if the definition of the credential's CType is not given.
* @param options.loadCTypes A function to load CType definitions that are not in cTypes. Defaults to using the {@link newCachingCTypeLoader | CachingCTypeLoader}. If set to false or undefined, no additional CTypes will be loaded.
*
* @throws {Error} If the credential type does not contain a valid CType id
* @throws {Error} If required CType definitions cannot be loaded
* @throws {Error} If claims do not follow the expected CType format
* @throws {Error} If referenced CTypes in nested structure cannot be fetched from the blockchain
* @throws {Error} If validation fails against the CType schema
*/
export async function validateSubject(
{
Expand All @@ -344,7 +406,7 @@ export async function validateSubject(
}: Pick<KiltCredentialV1, 'credentialSubject' | 'type'>,
{
cTypes = [],
loadCTypes = cachingCTypeLoader,
loadCTypes = newCachingCTypeLoader(),
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason for this change?

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 at (i missed it that it already exists): 77b9c3d

}: { cTypes?: ICType[]; loadCTypes?: false | CTypeLoader } = {}
): Promise<void> {
// get CType id referenced in credential
Expand All @@ -354,6 +416,7 @@ export async function validateSubject(
if (!credentialsCTypeId) {
throw new Error('credential type does not contain a valid CType id')
}

// check that we have access to the right schema
let cType = cTypes?.find(({ $id }) => $id === credentialsCTypeId)
if (!cType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to somehow handle both the ctypes passed in as the cTypes argument, and the loadCtypes function, if there is any. I suggest we create a combined ctype loader function from both, which first checks if the cType whose id is being looked up is contained in the cTypes array, and if not, forwards the id to the loadCtypes function, which then tries to look it up.

Hint: checking if we have a CType already in cTypes can be done quicker if we transform the array into a Map of ctype id to ctype definition once.

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 am working on a new commit for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is still tbd

Expand Down Expand Up @@ -385,6 +448,43 @@ export async function validateSubject(
[key.substring(vocab.length)]: value,
}
}, {})
// validates against CType (also validates CType schema itself)
CType.verifyClaimAgainstSchema(claims, cType)

// Connect to blockchain
const api = Kilt.ConfigService.get('api')
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be needed, we have the loadCTypes argument to load ctypes

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 at: 77b9c3d


// Bizim eklediğimiz doğrulama mantığı
const isNested = cTypeTypeFinder(cType)

if (!isNested) {
await CType.verifyClaimAgainstNestedSchemas(
cType,
[],
claims
)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

skip this step; call extractUniqueReferences directly, pass each ctype id to the ctype loader, then pass the resulting definitions to verifyClaimAgainstNestedSchemas. If there are no ctype references in the ctype, this result in an empty array, just like here.

const references = extractUniqueReferences(cType)
const referencedCTypes: ICType[] = []

for (const ref of references) {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest using a Promise.all here to reduce the code complexity and make this more efficient.

Additionally, change this from a for..loop with the await inside to a map. Using the map const you can use the Promise.all

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, use array methods here please. E.g., await Promise.all(Array.from(references).map(ctypeId => loadCTypes(ctypeId)))

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 at: 77b9c3d

try {
const referencedCType = await CType.fetchFromChain(ref as any)
if (referencedCType.cType) {
referencedCTypes.push(referencedCType.cType)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

again, use the loadCTypes argument. If it can't be found, throw an error saying that the ctype contains a reference to another ctype which cannot be resolved.

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 at: 77b9c3d

} catch (error) {
console.error(`Failed to fetch CType for reference ${ref}:`, error)
throw new Error(`Failed to fetch CType from chain: ${ref}`)
}
}

if (referencedCTypes.length === references.size) {
await CType.verifyClaimAgainstNestedSchemas(
cType,
referencedCTypes,
claims
)
} else {
throw new Error("Some referenced CTypes could not be fetched")
}
}
}
Loading