MCP Fixes & Improvements#10
Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR introduces a crucial disclaimer mechanism when a disposable private key is used, leveraging MCP elicitation to surface warnings to the client. It also extends the resource catalog with comprehensive Markdown documentation for C2D algorithm authoring. The design is solid and improves both developer experience and security awareness.
Comments:
• [INFO][bug] It is a good practice to pass any potential arguments to the previous handler and await it in case it performs asynchronous operations. This prevents subtle race conditions and ensures full compatibility if the underlying MCP SDK evolves or if multiple handlers are chained.
- server.server.oninitialized = () => {
- previous?.()
+ server.server.oninitialized = async (...args: any[]) => {
+ if (previous) await previous(...args)• [INFO][style] Good clear warning structure. Using MCP elicitation to present security warnings to users directly in the client context is an excellent utilization of the protocol features.
|
| Filename | Overview |
|---|---|
| src/tools/p2pProviderTools.ts | Redesigns create_auth_token to ephemeral/privateKey flow with key-scrubbing error handling; adds compute guidance strings to tool descriptions; reworks computeAssetList to document bare-fileObject datasets. Minor internal guidance contradiction on whether getPersistentStorageFileObject is required. |
| src/tools/evmContractTools.ts | Adds get_erc20_token_info tool: reads decimals/symbol/name from any ERC-20 via read-only contract call; optionally formats a raw amount string using BigInt+formatUnits. All errors caught and returned as isError. |
| src/utils/c2dAlgorithmAuthoring.ts | New markdown resource documenting the rawcode + prebuilt image C2D path; covers image tag catalog, algorithm object shape, filesystem contract, constraints, and free-compute auth. |
| src/tools/p2pSchemas.ts | Adds STORAGE_OBJECT_SHAPE_GUIDE, OCEAN_RECOMMENDED_NODES, P2P_RECOMMENDED_NODES_GUIDE, EPHEMERAL_CONSUMER_KEY_DISCLAIMER, P2P_COMPUTE_PAYMENT_GUIDE, and P2P_COMPUTE_POLLING_GUIDE constants; guidance is detailed and internally consistent. |
| src/tools/registerDocsTools.ts | Migrates Sepolia to Base (8453), adds COMPY alongside USDC as accepted fee tokens, updates write_c2d_algo workflow steps; relaxes validateAlgoCode to warnings; write_c2d_algo step 5 lists checksum in the algorithm object shape contradicting the omit-checksum guidance elsewhere. |
| src/test/unit/tools/createAuthToken.test.ts | New unit tests cover ephemeral path, privateKey path, no-source rejection, dual-source rejection, invalid key rejection, and downstream mint failure — all asserting key non-exposure. |
| src/clients/nodeClient.ts | Adds createAuthTokenWithSigner method that wraps generateAuthToken with a Signer; cleanly delegates nonce fetch and signing to ocean.js internally. |
| src/resources/resourceCatalog.ts | Registers the new c2d-algorithm-authoring builtin resource URI and serves its markdown content correctly. |
| src/prompts/registerPrompts.ts | Updates payment error diagnostics and env template to reference Base fee tokens (USDC + COMPY) and removes Sepolia from default RPCS config. |
Sequence Diagram
sequenceDiagram
participant U as LLM Agent
participant T as create_auth_token
participant W as ethers Wallet
participant N as NodeClient
alt ephemeral: true
U->>T: "nodeId, ephemeral=true"
T->>W: Wallet.createRandom()
W-->>T: ephemeralWallet
T->>N: createAuthTokenWithSigner(node, ephemeralWallet)
N-->>T: JWT string
T-->>U: token, consumerAddress, privateKey, disclaimer
else privateKey provided
U->>T: nodeId, privateKey
T->>W: new Wallet(privateKey)
W-->>T: wallet address only
T->>N: createAuthTokenWithSigner(node, wallet)
N-->>T: JWT string
T-->>U: token, consumerAddress only
end
Note over U,N: Paid compute flow after auth
U->>N: getComputeEnvironments
N-->>U: envs with minJobDuration
U->>N: initializeCompute
N-->>U: payment amount and minLockSeconds
U->>T: get_erc20_token_info with rawAmount
T-->>U: formattedAmount and symbol
U->>N: computeStart with authToken
loop poll every 5-10s
U->>N: computeStatus
N-->>U: C2DStatusNumber
end
U->>N: getComputeResult
Reviews (12): Last reviewed commit: "clearer guidance for buckets" | Re-trigger Greptile
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR introduces excellent improvements to C2D algorithm authoring documentation, adds a new get_erc20_token_info tool for proper token denomination, and refactors create_auth_token to handle wallet signing directly on the server side instead of delegating signature construction to the client. The security precautions taken to explicitly suppress and sanitize exceptions to prevent private key leakage are top-notch. LGTM!
Comments:
• [INFO][security] Excellent job meticulously avoiding the exposure of the user's private key in both ethers.js parsing errors and downstream node API errors. Masking the raw error here is a very strong security practice!
• [INFO][bug] Since rawAmount represents base token units and must be an integer, you could enforce this at the Zod schema level. This provides a clearer framework-level validation error if an LLM incorrectly attempts to pass a float (e.g., "1.5"), which would otherwise cause BigInt() to throw a generic error later.
rawAmount: z
.string()
+ .regex(/^\d+$/, 'Must be an integer string')
.optional()
.describe(• [INFO][style] While validating the exclusivity of ephemeral and privateKey in the handler works perfectly, you can optionally enforce this using Zod's .refine() at the schema level to let the MCP server handle the rejection automatically.
- }
+ }).refine(data => [data.ephemeral === true, !!data.privateKey].filter(Boolean).length === 1, {
+ message: 'Provide exactly one of: ephemeral or privateKey'
+ })|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR excellently updates the default configurations and workflows from Sepolia to Base, introduces C2D algorithm authoring guides, adds an ERC20 token info utility, and significantly improves prompt/schema documentation for LLM interactions. The refactoring of the auth token creation to securely handle private keys with strong error-masking is highly commendable. LGTM!
Comments:
• [INFO][security] Excellent job catching the Wallet instantiation and masking the error to prevent the private key from being accidentally echoed back in the exception message. This is a very strong security practice when passing credentials through LLMs/MCP interfaces.
• [INFO][other] The implementation of get_erc20_token_info correctly utilizes Promise.all for performance and provides a much-needed utility for formatting token units cleanly in the client.
• [INFO][style] Refining the algorithm validation checks to account for usesDataInputs and usesPersistentStorage, and demoting the DIDS check to a warning, significantly improves the developer experience for cases that do not strictly require it.
|
LGTM |
Fixes # .
Changes proposed in this PR:
ocean://docs/c2d-algorithm-authoringresource + reworked workflow for inlinemeta.rawcodeagainst prebuiltoceanprotocol/c2d_examplesimages (no Docker build); relaxedvalidate_algo_structureto warningscreate_auth_tokenredesign: takesephemeralorprivateKeyinstead ofcompleteSignature, returnsconsumerAddress, and never echoes privatekeys (+ unit tests)
8453) and added COMPY alongside USDC as a fee tokenfileObjectshape guides; documented bare-fileObjectdatasetsget_erc20_token_infotool to denominate raw token amounts before showing costsPaid-compute flow:
computeStartnow directs users without a token to the dashboard (deposit + authorize) rather than minting one, and clarifiesmaxJobDuration/validUntilare container run time, not the escrow lockauthToken) and the/data/persistentStorage/<bucketId>/<fileName>mount pathPRIVATE_KEYwarning now explains it's only the libp2p server identity, not the key that funds or authorizesjobs