feat: nested ctype val #919

aybarsayan · 2024-11-20T10:28:05Z

fixes KILTProtocol/ticket#3632

Added nested CType validation support to validateSubject
Implemented automatic reference detection and blockchain fetching
Added support to handle both simple and complex CType structures
Updated tests for new functionality

Note: Improves credential validation by handling nested CTypes automatically.

How to test:

Please provide a brief step-by-step instruction.
If necessary provide information about dependencies (specific configuration, branches, database dumps, etc.)

Step 1
Step 2
etc.

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

Dudleyneedham

In general, this is bad practice to create a Named function into another Named function unless you are using it multiple times.

There is a time and place for doing it, but it isn't here.

If you are worried about the typing you can cast or try another approach.

Dudleyneedham · 2024-11-25T14:56:49Z

packages/credentials/src/V1/KiltCredentialV1.ts

+    const references = extractUniqueReferences(cType)
+    const referencedCTypes: ICType[] = []
+
+    for (const ref of references) {


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

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

Fixed at: 77b9c3d

Dudleyneedham · 2024-11-25T15:03:16Z

packages/credentials/src/V1/KiltCredentialV1.ts

+// 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)
+}


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

})

}

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.

Fixed at: 77b9c3d

Dudleyneedham · 2024-11-25T15:04:35Z

packages/credentials/src/V1/KiltCredentialV1.ts

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


As above, change it.

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> {

Fixed at: 77b9c3d

rflechtner · 2024-11-28T16:41:12Z

packages/credentials/src/V1/KiltCredentialV1.ts

@@ -7,6 +7,7 @@

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


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.

Fixed at: 77b9c3d

rflechtner · 2024-11-28T16:51:24Z

packages/credentials/src/V1/KiltCredentialV1.ts

+// 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)
+}


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.

rflechtner · 2024-11-28T16:53:42Z

packages/credentials/src/V1/KiltCredentialV1.ts

+function extractUniqueReferences(cType: ICType): Set<string> {
+  const references = new Set<string>()
+
+  function processValue(value: any) {


Don't use any, use unknown

fixed at: 77b9c3d

rflechtner · 2024-11-28T16:56:37Z

packages/credentials/src/V1/KiltCredentialV1.ts

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


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> {

rflechtner · 2024-11-28T16:57:15Z

packages/credentials/src/V1/KiltCredentialV1.ts

@@ -344,7 +406,7 @@ export async function validateSubject(
  }: Pick<KiltCredentialV1, 'credentialSubject' | 'type'>,
  {
    cTypes = [],
-    loadCTypes = cachingCTypeLoader,
+    loadCTypes = newCachingCTypeLoader(),


what's the reason for this change?

Fixed at (i missed it that it already exists): 77b9c3d

rflechtner · 2024-11-28T16:59:34Z

packages/credentials/src/V1/KiltCredentialV1.ts

+  // Connect to blockchain
+  const api = Kilt.ConfigService.get('api')


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

fixed at: 77b9c3d

rflechtner · 2024-11-28T17:03:37Z

packages/credentials/src/V1/KiltCredentialV1.ts

+  // Bizim eklediğimiz doğrulama mantığı
+  const isNested = cTypeTypeFinder(cType)
+
+  if (!isNested) {
+    await CType.verifyClaimAgainstNestedSchemas(
+      cType,
+      [],
+      claims
+    )
+  } else {


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.

rflechtner · 2024-11-28T17:07:47Z

packages/credentials/src/V1/KiltCredentialV1.ts

+    const references = extractUniqueReferences(cType)
+    const referencedCTypes: ICType[] = []
+
+    for (const ref of references) {


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

rflechtner · 2024-11-28T17:10:48Z

packages/credentials/src/V1/KiltCredentialV1.ts

+        const referencedCType = await CType.fetchFromChain(ref as any)
+        if (referencedCType.cType) {
+          referencedCTypes.push(referencedCType.cType)
+        }


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.

Fixed at: 77b9c3d

rflechtner · 2024-11-28T17:21:22Z

packages/credentials/src/V1/KiltCredentialV1.ts

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


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.

I am working on a new commit for this.

this is still tbd

rflechtner

Looking way better already, but there's still a few things that need to be tackled

rflechtner · 2024-12-03T11:03:50Z

packages/credentials/src/V1/KiltCredentialV1.ts

+    const referencedCTypes = await Promise.all(
+      Array.from(references).map(async (ref) => {
+        try {
+          const referencedCType = await cachingCTypeLoader(ref as any)


please use the function passed as the loadCtypes argument

Fixed at: bbfced2

rflechtner · 2024-12-03T11:05:26Z

packages/credentials/src/V1/KiltCredentialV1.ts

+    // eslint-disable-next-line no-console
+    console.error('Validation error:', error)


we don't use console statements in library code, that's why the lint rule exists

Fixed at: bbfced2

rflechtner · 2024-12-03T11:05:51Z

packages/credentials/src/V1/KiltCredentialV1.ts

+          // eslint-disable-next-line no-console
+          console.error(`Failed to fetch CType for reference ${ref}:`, error)


we don't use console statements in library code, that's why the lint rule exists

Fixed at: bbfced2

rflechtner · 2024-12-03T11:06:21Z

packages/credentials/src/V1/KiltCredentialV1.ts

+// Check recursively if a value has references
+const hasRef = (value: unknown): boolean => {
+  if (typeof value !== 'object' || value === null) {
+    return false
+  }
+
+  if ('$ref' in (value as Record<string, unknown>)) {
+    return true
+  }
+
+  if (Array.isArray(value)) {
+    return value.some((item) => hasRef(item))
+  }
+
+  return Object.values(value as Record<string, unknown>).some((v) => hasRef(v))
+}


you're not using this function any more, are you?

Yes forget to remove, fixed at: bbfced2

rflechtner · 2024-12-03T11:12:09Z

packages/credentials/src/V1/KiltCredentialV1.ts

+    if (Array.isArray(value)) {
+      value.forEach(processValue)
+    } else {
+      Object.values(objValue).forEach(processValue)


the point of passing down the set of references to extractUniqueReferences was that you can call extractUniqueReferences in recursion instead of an inner function. You don't necessarily need to do it that way, but if you're not using this parameter, you can also just pull it back into the function as a normal variable.

Fixed at: bbfced2

rflechtner · 2024-12-03T11:13:01Z

packages/credentials/src/V1/KiltCredentialV1.ts

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


this is still tbd

rflechtner · 2024-12-04T09:57:47Z

packages/credentials/src/V1/KiltCredentialV1.ts

+  const references = extractUniqueReferences(cType)
+
+  const referencedCTypes = await Promise.all(
+    Array.from(references).map(async (ref) => {
+      if (typeof loadCTypes !== 'function') {
+        throw new Error(
+          `The definition for this credential's CType ${ref} has not been passed to the validator and CType loading has been disabled`
+        )
+      }
+      return loadCTypes(ref as any)
+    })
+  )


Sorry that I'm only realising that now, but we have two more issues here that need to be addressed:

There is a maximum recursion depth for synchronous code in JS, which means that deeply nested structures could end up triggering a max recursion depth reached error

The CTypes that we fetch could also contain references, so we need to check their definitions too and fetch any CTypes that they depend on

In light of these issues I think it's a good idea to refactor extractUniqueReferences so that it is async and also loads the ctype definitions directly. This function could be named loadNestedCTypeDefinitions and would

Take a CType definition cType and a CTypeLoader cTypeLoader as parameters

Initialise an empty Set fetchedCTypeIds

Initialise an empty Set fetchedCTypeDefinitions

Define a subroutine processCTypeProperties which accepts ICType["properties"] and:

iterates over all properties in the object

If the property value is an array, call and await processCTypeProperties on each item in it and continue

If the property key is $ref, extract the CType id from it

If the extracted CType id is not yet in fetchedCTypeIds, push the id to it, then call cTypeLoader passing the id as an argument

await the Promise returned by the cTypeLoader; if it's a CType, call processCTypeProperties on its properties; else, throw an error

calls processCTypeProperties on the CType properties and awaits its completion

returns fetchedCTypeDefinitions

feat: nested ctype val

2a795b7

aybarsayan requested a review from rflechtner November 20, 2024 10:28

Dudleyneedham reviewed Nov 25, 2024

View reviewed changes

rflechtner requested changes Nov 28, 2024

View reviewed changes

aybarsayan added 3 commits December 2, 2024 22:33

fix: review changes

77b9c3d

fix: linter added

9e2596e

fix linter2

d851381

rflechtner reviewed Dec 3, 2024

View reviewed changes

fix: review2

bbfced2

rflechtner reviewed Dec 4, 2024

View reviewed changes

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

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

		// eslint-disable-next-line no-console
		console.error('Validation error:', error)

		// eslint-disable-next-line no-console
		console.error(`Failed to fetch CType for reference ${ref}:`, error)

feat: nested ctype val #919

Are you sure you want to change the base?

feat: nested ctype val #919

Conversation

aybarsayan commented Nov 20, 2024

fixes KILTProtocol/ticket#3632

How to test:

Checklist:

Dudleyneedham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rflechtner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment