Conversation
chore: add example metadata
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request introduces support for DDOs (Decentralized Data Objects) version 5.0.0, enabling Self-Sovereign Identity (SSI) policy flows for asset interactions like download and compute. It includes new CLI arguments for specifying service IDs, refactors core command logic to be DDO version-aware, and adds new modules for interacting with SSI wallets and policy servers. Several dependencies have also been updated.
Comments:
• [INFO][style] Consider clarifying the example DID format for SSI_WALLET_DID. While did:example is a valid placeholder for a DID method, a more complete example like did:example:your-specific-did might be clearer for users to understand what format to expect for a 'DID'.
• [INFO][style] For serviceId in the download command, the argument is [serviceId] (optional positional) and also --service <serviceId> (optional named option). The action handler uses options.service || serviceId. This is fine, but for consistency, you might consider always passing undefined if no service ID is provided by either option, rather than an empty string, as it's a string | undefined type in commands.ts. However, current commands.ts logic handles empty strings by defaulting, so it's not strictly a bug.
• [INFO][style] Similar to the download command, ensure a clear distinction and handling for serviceIds and algoServiceId being undefined (if not provided) versus empty strings. The current logic options.services ?? serviceIds ?? '' will always result in a string, which is then parsed. This works, but direct undefined for optional parameters can sometimes be more semantically clear in downstream functions.
• [WARNING][bug] Accessing services[0].files and indexedMetadata.nft.name (and similar in publishAlgo) assumes that services array is not empty and indexedMetadata.nft exists. While DDOs are expected to have at least one service and NFT metadata, it's safer to add a check (e.g., services?.[0]?.files) or ensure DDO validation strictly enforces these fields.
• [WARNING][bug] Accessing services[0].id without checking if the services array is empty could lead to a runtime error if a malformed DDO is somehow processed. Consider adding a check for services.length > 0 before accessing services[0] or adding a nullish coalescing operator services?.[0]?.id.
• [WARNING][bug] Accessing ddos[0].services[0].serviceEndpoint without checking ddos.length and services.length can lead to runtime errors if ddos is empty or the first DDO has no services. A check like if (ddos.length > 0 && services.length > 0) is recommended.
• [WARNING][bug] The logic ddos[0].services[0].serviceEndpoint is still present, which shares the same potential issue as line 277. Please ensure proper checks for array length before accessing elements.
• [INFO][style] The conditional logic for resolving chosenAlgoServiceId involves checking typeof algoServiceIdInput === 'string' && algoServiceIdInput.trim().length > 0. This is good, but consider if algoServiceIdInput could also be undefined if passed directly from cli.ts (depending on how commander.js handles omitted optional positional arguments). The current commands.ts side explicitly checks and handles string values including ''. This is more of a minor style consistency suggestion.
• [INFO][other] The error message here is very helpful and explicit, guiding the user on how to provide a valid algoServiceId. Good job.
• [INFO][style] The conditional logic for resolving chosenServiceId from inputServices mirrors that of algoServiceId. Same comment applies regarding undefined vs '' and explicit checks for inputServices.length > 0.
• [WARNING][bug] Similar to line 209 and 277, accessing ddos[0].services[0].serviceEndpoint without checking ddos.length and services.length can cause errors. Please add appropriate checks.
• [INFO][style] The chosenAlgoServiceId resolution and validation logic is duplicated from initializeCompute. Consider extracting this common logic into a helper function to improve maintainability and prevent inconsistencies.
• [WARNING][bug] The algoServiceIndex lookup servicesAlgo.findIndex((s: any) => s.id === chosenAlgoServiceId) might return -1 if chosenAlgoServiceId is not found, leading to a negative index being passed to downstream functions. While an error is logged, returning early or throwing an error here would prevent silent failures or unexpected behavior in subsequent calls. (It currently returns, which is good.)
• [INFO][style] Similar to previous comments, the conditional logic for resolving chosenServiceId from inputServices is duplicated. Consider extracting this into a helper function.
• [WARNING][bug] The chosenServiceIndex lookup for dataset services also needs to handle the -1 case to prevent invalid indices from being passed downstream. It currently returns early, which is good.
• [WARNING][bug] Accessing ddos[0].services[0].serviceEndpoint without checking ddos.length and services.length can lead to runtime errors. Please add appropriate checks.
• [INFO][style] The chosenAlgoServiceId resolution and validation logic is duplicated from initializeCompute. Consider extracting this common logic into a helper function to improve maintainability and prevent inconsistencies.
• [WARNING][bug] The algoServiceIndex lookup for freeComputeStart also needs to handle the -1 case to prevent invalid indices from being passed downstream. It currently returns early, which is good.
• [INFO][style] Similar to previous comments, the conditional logic for resolving chosenServiceId from inputServices is duplicated. Consider extracting this into a helper function.
• [WARNING][bug] The chosenServiceIndex lookup for dataset services also needs to handle the -1 case to prevent invalid indices from being passed downstream. It currently returns early, which is good.
• [WARNING][bug] Accessing algoAsset.services[0].id and algoAsset.services[0].serviceEndpoint without checking if the services array is empty could lead to a runtime error. Add a check for services.length > 0 or use nullish coalescing operators.
• [INFO][style] The connectToSSIWallet function currently throw error.response. It's generally better to throw a custom error or at least a standard Error object with a message derived from error.response.data or error.response.status for clearer error handling upstream, especially for non-2xx HTTP responses where error.response contains more details.
• [INFO][style] The getPolicyServerOBJ function has extensive error handling. The catch block console.error('getPolicyServerOBJ error:', error) could be more specific. If error already has a message, throw new Error(getPolicyServerOBJ failed: ${error.message}) is good. Otherwise, just rethrowing error (if it's an Error object) or wrapping it (new Error('getPolicyServerOBJ failed', { cause: error })) would preserve the original error context. For now, it's acceptable but could be slightly improved for debugging.
• [WARNING][security] The use of eval(jsonMatch[1]) to parse JSON in a test file, while less risky than in production, is still a potential security vulnerability if the jsonMatch[1] string could ever be influenced by external input. It's best practice to use JSON.parse() for parsing JSON strings. While the test setup ensures controlled input, eval has broader capabilities than just JSON parsing and should generally be avoided.
Fixes # .
Changes proposed in this PR:
need to set also env for v5 like: