Skip to content

ddoJs for v5#131

Open
AdriGeorge wants to merge 35 commits intomainfrom
feat/ddojs-commands
Open

ddoJs for v5#131
AdriGeorge wants to merge 35 commits intomainfrom
feat/ddojs-commands

Conversation

@AdriGeorge
Copy link
Collaborator

@AdriGeorge AdriGeorge commented Sep 15, 2025

Fixes # .

Changes proposed in this PR:

  • integrated policy server
  • allow publish v5 with ddo.js
  • download v5 with ddo.js throw command: npm run cli download did path serviceId
  • start free compute for v5 with ddo.js
  • start paid compute for v5 with ddo.js
  • publish algo v5 with ddo.js
  • allow/disallow algo v5 with ddo.js
  • added metadata example for dataset and algo v5
  • edit download to accept optionally serviceId
  • edit startFreeCompute/startCompute to accept optionally serviceIds and algoServiceId
  • README
  • update tests

need to set also env for v5 like:

  • export SSI_WALLET_API=""
  • export SSI_WALLET_ID=""
  • export SSI_WALLET_DID="" (this is optional, if not settet, will use the first one)

@AdriGeorge AdriGeorge marked this pull request as ready for review September 19, 2025 07:40
@alexcos20
Copy link
Member

/run-security-scan

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

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.

@alexcos20 alexcos20 self-requested a review February 6, 2026 12:02
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

lgtm

@alexcos20 alexcos20 added the doNotMerge not merge yet label Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doNotMerge not merge yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants