-
Notifications
You must be signed in to change notification settings - Fork 27
fix: nested ctype loading #931
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
Changes from all commits
2a795b7
77b9c3d
9e2596e
d851381
bbfced2
a97c066
1adad9d
b0fbaf2
bff81a9
6f6ca97
7751f5b
304844e
85ec86b
57c46a7
8ad84da
e5298ee
3d166dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,15 @@ | |
* found in the LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import { ICType } from '@kiltprotocol/types' | ||
import type { ICType } from '@kiltprotocol/types' | ||
import { SDKErrors } from '@kiltprotocol/utils' | ||
|
||
import { fetchFromChain } from './CType.chain.js' | ||
import { isICType, verifyDataStructure } from './CType.js' | ||
|
||
export type CTypeLoader = (id: ICType['$id']) => Promise<ICType> | ||
|
||
const loadCType: CTypeLoader = async (id) => { | ||
const chainCTypeLoader: CTypeLoader = async (id) => { | ||
Dudleyneedham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return (await fetchFromChain(id)).cType | ||
} | ||
|
||
|
@@ -20,10 +22,13 @@ const loadCType: CTypeLoader = async (id) => { | |
* Used in validating the credentialSubject of a {@link KiltCredentialV1} against the Claim Type referenced in its `type` field. | ||
* | ||
* @param initialCTypes An array of CTypes with which the cache is to be initialized. | ||
* @returns A function that takes a CType id and looks up a CType definition in an internal cache, and if not found, tries to fetch it from the KILT blochchain. | ||
* @param cTypeLoader A basic {@link CTypeLoader} to augment with a caching layer. | ||
* Defaults to loading CType definitions from the KILT blockchain. | ||
* @returns A function that takes a CType id and looks up a CType definition in an internal cache, and if not found, tries to fetch it from an external source. | ||
*/ | ||
export function newCachingCTypeLoader( | ||
initialCTypes: ICType[] = [] | ||
initialCTypes: ICType[] = [], | ||
cTypeLoader = chainCTypeLoader | ||
): CTypeLoader { | ||
const ctypes: Map<string, ICType> = new Map() | ||
|
||
|
@@ -32,9 +37,69 @@ export function newCachingCTypeLoader( | |
}) | ||
|
||
async function getCType(id: ICType['$id']): Promise<ICType> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Randomly adding more soundness checks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved them from the code for loading nested ctype definitions, as they seemed misplaced there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok fair enough. |
||
const ctype: ICType = ctypes.get(id) ?? (await loadCType(id)) | ||
let ctype = ctypes.get(id) | ||
if (ctype) { | ||
return ctype | ||
} | ||
ctype = await cTypeLoader(id) | ||
verifyDataStructure(ctype) | ||
if (id !== ctype.$id) { | ||
throw new SDKErrors.CTypeIdMismatchError(ctype.$id, id) | ||
} | ||
ctypes.set(ctype.$id, ctype) | ||
return ctype | ||
} | ||
return getCType | ||
} | ||
|
||
/** | ||
* Recursively traverses a (nested) CType's definition to load definitions of CTypes referenced within. | ||
* | ||
* @param cType A (nested) CType containg references to other CTypes. | ||
* @param cTypeLoader A function with which to load CType definitions. | ||
* @returns An array of CType definitions which were referenced in the original CType or in any of its composite CTypes. | ||
*/ | ||
export async function loadNestedCTypeDefinitions( | ||
cType: ICType, | ||
cTypeLoader: CTypeLoader | ||
): Promise<ICType[]> { | ||
const fetchedCTypeIds = new Set<string>() | ||
const fetchedCTypeDefinitions: ICType[] = [] | ||
|
||
// Don't fetch the original CType | ||
fetchedCTypeIds.add(cType.$id) | ||
|
||
async function extractRefsFrom(value: unknown): Promise<void> { | ||
if (typeof value !== 'object' || value === null) { | ||
return | ||
} | ||
|
||
if ('$ref' in value) { | ||
const ref = (value as { $ref: unknown }).$ref | ||
if (typeof ref === 'string' && ref.startsWith('kilt:ctype:')) { | ||
const cTypeId = ref.split('#/')[0] as ICType['$id'] | ||
|
||
if (!fetchedCTypeIds.has(cTypeId)) { | ||
fetchedCTypeIds.add(cTypeId) | ||
const referencedCType = await cTypeLoader(cTypeId) | ||
|
||
if (isICType(referencedCType)) { | ||
fetchedCTypeDefinitions.push(referencedCType) | ||
} else { | ||
throw new Error(`Failed to load referenced CType: ${cTypeId}`) | ||
} | ||
|
||
await extractRefsFrom(referencedCType.properties) | ||
} | ||
} | ||
return | ||
} | ||
|
||
// Process all values in the object. Also works for arrays | ||
await Promise.all(Object.values(value).map(extractRefsFrom)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had asked chat about this. Not sure if we care to much about proformance. However, it gave me an interesting insight into this await Promise.all(Object.values(value).map(extractRefsFrom))
for (const subValue of Object.values(value)) {
await extractRefsFrom(subValue)
} This prevents too many concurrent fetches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bizarre that chad thinks performance would be better if we process them sequentially. Async code exists exactly for this use case, where it doesn't make much sense to block execution while waiting for the full node to return historic blocks. In case there are issues with making too many requests at the same time, I'd recommend rate limiting on the ctype loader side, but not here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is strange. There wasn't any issues on my side, but I like to run it through chat afterwards. Seeing what interesting things it comes up with. I sent the test results. It basically a pointless comment without the context of what we are doing in the project. |
||
} | ||
|
||
await extractRefsFrom(cType.properties) | ||
|
||
return fetchedCTypeDefinitions | ||
} | ||
Dudleyneedham marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.