index escrow events#1378
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 successfully introduces indexing and querying for Ocean Protocol Escrow events. The implementation is well-architected, cleanly extending the existing Command Handler, Indexer, and Database patterns (both Elasticsearch and Typesense). Excellent use of data normalization and robust parsing logic for blockchain events.
Comments:
• [INFO][performance] When constructing Typesense filter_by queries, it's a recommended best practice to wrap string values in backticks to prevent syntax errors if the string ever contains spaces, hyphens, or other special characters. While the fields currently being filtered (addresses, hashes) are typically safe hex strings, adding backticks improves query robustness.
- .map(([field, value]) => `${field}:=${value}`)
+ .map(([field, value]) => `${field}:=${typeof value === 'string' ? `\`${value}\`` : value}`)• [WARNING][other] By default, Typesense indexes all string fields for searching and enforces a maximum length limit (often 2048 bytes per field) on indexed strings. The proof field from the Escrow Claimed event could potentially contain a large hex string exceeding this limit, which would cause record insertion to fail. Since it is highly unlikely you will need to search/filter by the proof value itself, consider disabling indexing for this field to avoid length restrictions.
- { name: 'proof', type: 'string', optional: true },
+ { name: 'proof', type: 'string', optional: true, index: false },• [INFO][style] Great job utilizing safe parsing helper functions (addr and num) to consistently handle missing arguments, enforce lowercase addresses, and prevent precision loss with uint256 values. The switch-case mapping for different escrow events is also very clean.
|
/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 PR implements escrow event indexing and querying, which cleanly extends the current architecture. It appropriately segregates database interactions and properly implements pagination. However, there is a NoSQL injection risk in the Typesense query builder and potential runtime crashes in the command handler when processing untrusted inputs.
Comments:
• [ERROR][security] User inputs (e.g., query string parameters) are passed directly into backticks within Typesense's filter_by syntax. If an attacker includes backticks in their payload, it breaks the string literal boundaries, resulting in a NoSQL injection vulnerability. Backticks should be stripped or escaped from the input values.
- typeof value === 'string' ? `${field}:=\`${value}\`` : `${field}:=${value}`
+ typeof value === 'string' ? `${field}:=\`${value.replace(/`/g, '')}\`` : `${field}:=${value}`• [WARNING][bug] P2P commands can send loosely structured JSON. If task.payer, task.payee, or task.token are provided as objects or arrays instead of strings, calling .toLowerCase() will throw a TypeError and cause an unhandled runtime error. Defensively check the types before calling string prototype methods.
- payer: task.payer ? task.payer.toLowerCase() : undefined,
- payee: task.payee ? task.payee.toLowerCase() : undefined,
- token: task.token ? task.token.toLowerCase() : undefined,
+ payer: typeof task.payer === 'string' ? task.payer.toLowerCase() : undefined,
+ payee: typeof task.payee === 'string' ? task.payee.toLowerCase() : undefined,
+ token: typeof task.token === 'string' ? task.token.toLowerCase() : undefined,• [INFO][performance] Good call using index: false for the proof field. This successfully avoids hitting Typesense's indexed-field length limitation for large cryptographic hex strings and preserves memory space.
|
/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 Escrow event indexing functionality. It successfully sets up Database schemas, an event processor, a P2P command handler, and the relevant API routes. Overall, the implementation is high-quality, defensively coded, and integrates well with the current architecture. A few minor robustness improvements are suggested to prevent edge-case TypeErrors and handle potentially malformed inputs.
Comments:
• [WARNING][bug] The addr helper uses optional chaining for toString() but directly calls .toLowerCase(). If v is null or undefined, v?.toString() evaluates to undefined, and calling .toLowerCase() on it will throw a TypeError. Although ethers usually guarantees the presence of event fields, it's safer to properly guard against undefined.
-const addr = (v: any): string => v?.toString().toLowerCase()
+const addr = (v: any): string | undefined => v ? v.toString().toLowerCase() : undefined• [INFO][security] Since GetEscrowEventsCommand can be sent over P2P directly as a JSON payload, an attacker could theoretically supply objects or arrays for eventType, jobId, or txId (e.g., jobId: { foo: 'bar' }). This could cause syntax errors in Typesense or malformed queries in Elasticsearch. Although these errors are currently caught and safely handled (returning 500 or an empty array), strictly typing them as strings is more robust and consistent with how payer, payee, and token are handled.
- eventType: task.eventType,
+ eventType: typeof task.eventType === 'string' ? task.eventType : undefined,
payer: typeof task.payer === 'string' ? task.payer.toLowerCase() : undefined,
payee: typeof task.payee === 'string' ? task.payee.toLowerCase() : undefined,
token: typeof task.token === 'string' ? task.token.toLowerCase() : undefined,
- jobId: task.jobId,
- txHash: task.txId
+ jobId: typeof task.jobId === 'string' ? task.jobId : undefined,
+ txHash: typeof task.txId === 'string' ? task.txId : undefined• [INFO][security] Excellent defensive programming here by using backticks and explicitly stripping any backticks from user input. This safely mitigates any risk of Typesense filter query syntax injection.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
| Filename | Overview |
|---|---|
| src/components/Indexer/processors/EscrowEventProcessor.ts | New processor that decodes all 6 Escrow events using the contract ABI, applies an address guard against the chain's Escrow contract to avoid false positives from generic event signatures, and upserts into the escrow collection. |
| src/components/core/handler/escrowHandler.ts | New query handler with eventType allowlist validation; a null return from search() is silently converted to an empty 200 response, masking DB errors from callers. |
| src/components/database/ElasticSchemas.ts | Adds escrow index schema; numeric-string fields (amount, expiry, maxLockedAmount, maxLockSeconds, maxLockCounts) and proof are mapped as text instead of keyword, which will break exact-match term queries on those fields (discussed in prior review thread). |
| src/components/database/ElasticSearchDatabase.ts | Adds ElasticsearchEscrowDatabase with standard CRUD and a term-query-based search; uses upsert semantics via index() with explicit ID which is appropriate for this append-only log. |
| src/components/database/TypesenseDatabase.ts | Adds TypesenseEscrowDatabase; filter_by values are properly backtick-quoted with backtick stripping to prevent injection, numeric fields use :=value syntax correctly. |
| src/components/httpRoutes/escrow.ts | New HTTP GET route; chainId is validated for NaN but offset/size are not (harmless — the DB layer falls back to defaults on NaN). eventType validation is delegated to the handler layer. |
| src/utils/constants.ts | Adds 6 Escrow event hashes, ESCROW_EVENTS allowlist array, and GET_ESCROW_EVENTS protocol command; all consistent with the rest of the constants file. |
| src/test/integration/escrow.test.ts | Integration test covering Deposit, Auth and Lock indexing plus handler query and pagination; uses waitForCondition correctly and skips gracefully when Escrow is not deployed on the test chain. |
Sequence Diagram
sequenceDiagram
participant Chain as Blockchain
participant Indexer as OceanIndexer
participant EP as EscrowEventProcessor
participant DB as EscrowDatabase(Typesense/ES)
participant HTTP as GET /api/services/escrow/events
participant Handler as EscrowEventsHandler
Chain->>Indexer: emits Escrow log (Auth/Lock/Claimed/Canceled/Deposit/Withdraw)
Indexer->>EP: processEvent(log, chainId, eventName)
EP->>EP: "verify log.address == escrowAddress"
EP->>EP: escrowInterface.parseLog(log)
EP->>EP: "build EscrowEvent record (id = txHash-logIndex)"
EP->>DB: create(record)
DB-->>EP: stored event
HTTP->>Handler: handle(GetEscrowEventsCommand)
Handler->>Handler: validate(eventType allowlist)
Handler->>DB: search(filters, offset, size)
DB-->>Handler: EscrowEvent[] or null
Handler-->>HTTP: 200 JSON array (or 503 on DB error)
Reviews (2): Last reviewed commit: "address pr comments" | Re-trigger Greptile
Fixes #1355
Changes proposed in this PR: