Conversation
alexcos20
requested review from
andreip136,
bogdanfazakas,
giurgiur99,
mihaisc and
ndrpp
as code owners
|
/run-security-scan |
alexcos20
left a comment
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 an additionalRoles configuration to the P2PProvider, enabling the advertising of new libp2p client protocols (e.g., /ocean/client/1.0.0 and role-specific variants). The handlers for these protocols are currently placeholders, signaling intent for future functionality without implementing it. Additionally, the CI workflow has been updated to trigger on all branches.
Comments:
• [WARNING][other] Changing branches from main to **' means the CI workflow will now run on all branches, including feature branches and potentially temporary ones. While this might be intended for broader testing, it can significantly increase CI usage and costs. Please confirm this change is intentional and consider if pull_request triggers are sufficient, or if a more specific branch pattern is desired (e.g., ['main', 'develop', 'release/**']).
• [INFO][other] Adding additionalRoles to P2PConfig is a good way to extend the system for future client types. It would be beneficial to add a bit more context in the PR description about the anticipated use cases for these roles, beyond the examples, to help reviewers understand the long-term vision.
• [WARNING][other] The handleProtocolCommands method is currently a no-op handler, as indicated by the comment. While advertising the protocol (/ocean/client/1.0.0) signals intent for future functionality, it's important to consider if advertising a protocol without immediate functionality could confuse other peers attempting to connect or interact with it. It might be useful to explicitly state in the PR description that this is purely for protocol advertising/discovery at this stage.
• [WARNING][style] The // eslint-disable-next-line no-unused-vars comment for remotePeer and remoteAddr is a temporary solution. When these variables are eventually used, the disable can be removed. For now, if they are truly not needed even for logging, consider omitting their destructuring to avoid the lint warning entirely, or perhaps adding a placeholder log if useful for debugging future implementations.
• [ERROR][bug] There appears to be an extra single quote in the protocol string when handling additionalRoles. It currently reads '/ocean/client/${role}/1.0.0' which would result in a protocol like '/ocean/client/dashboard/1.0.0'. It should likely be just ``/ocean/client/${role}/1.0.0 to match the format of the base client protocol.
fixed in 878b58b |
* p2p impl * parallel http and p2p * rename job * fix field blob * serialize string * validate ddo * revert decryptor url * revert to http for now * fix signatures * lint fix * decrypt p2p and refactor * cleanup * cleanup and refactroing * p2p warmup * find peer * move functions to class level * remove unused checks * async node creation * reuse same provider * Updating ComputeExamples.md * simplift to hex and log err msg * lint fix * revert bad conversion * enable tcp and new method * fix methods mismatch * use signer in tests * Updating ComputeExamples.md * try peer id * use contracts version main * export contracts version main * get compute result, not only by url * lint fix * signature if no auth token * remove lint unused vars * encypt docker registry auth p2p * rename env to NODE * rename to NODE_ENDPOINT, NODE is reserved * Updating CodeExamples.md * Updating ComputeExamples.md * push,fetch config and no tls * Release 7.0.0-next.0 * bump uint8arrays to esm supported * getmultiaddr from peerid * remove mdns * print ci barge start script logs * remove explicit contracts version * start without typesense * Release 7.0.0-next.1 * improve check * circuit relay * circuit relay provider * libp2p circuit + evict bad connections * rename methods * fix unused auth in initializecompute * fix test init compute * Updating ComputeExamples.md * Release 7.0.0-next.2 * replace uint8arrays lib with native * fix exports name in pacakge json * Revert "fix exports name in pacakge json" This reverts commit 278ef0f. * Release 7.0.0-next.3 * devdep libp2p + fix exports * Release 7.0.0-next.4 * fix circuit parsing * Release 7.0.0-next.5 * keep only esential files * Release 7.0.0-next.6 * get node jobs and get node status * add fromtimestamp http * use direct command for status * Release 7.0.0-next.7 * allow multiaddr dial * Release 7.0.0-next.8 * fix review comments * Release 7.0.0-next.9 * p2p store discovered nodes & add dockerregistryauth * lint fix * Release 7.0.0-next.10 * export libp2p node * Release 7.0.0-next.11 * fetchnodelogs p2p method * Release 7.0.0-next.12 * create auth token with signed messge * Release 7.0.0-next.13 * ci use 127.0.0.1 * use older branch * Revert "ci use 127.0.0.1" This reverts commit db531ab. * add optional stream to command (#2065) * add optional stream to command * run CI on all branches, not just main * expose client protocols (#2066) * expose client protocols --------- Co-authored-by: GitHub Actions Bot <> Co-authored-by: alexcos20 <alex.coseru@gmail.com>
3 participants
Changes proposed in this PR: