Conversation
WalkthroughAdds a new TrustlessWork feature: NestJS module with controller, service, DTOs, tests, and app wiring to call an external Trustless Work API; updates env example and dependencies; registers new entities/modules in AppModule and exposes health/config endpoints. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as TrustlessWorkController
participant Service as TrustlessWorkService
participant Config as ConfigService
participant HTTP as HttpService
participant API as Trustless Work API
Client->>Controller: POST /escrows/initialize
Controller->>Service: initializeEscrow(dto)
Service->>Config: read base URL / API key
Service->>HTTP: POST /escrows/{type}/deploy (payload)
HTTP->>API: HTTP request
API-->>HTTP: Response (contract_id / unsigned_xdr)
HTTP-->>Service: Response data
Service-->>Controller: { success, contract_id, unsigned_xdr }
Controller-->>Client: 200 OK
sequenceDiagram
autonumber
actor Client
participant Controller as TrustlessWorkController
participant Service as TrustlessWorkService
participant HTTP as HttpService
participant API as Trustless Work API
rect rgba(230,240,255,0.4)
Client->>Controller: GET /health
Controller->>Service: healthCheck()
Service->>HTTP: GET /health (with timeout)
HTTP->>API: Request
API-->>HTTP: 200 / status
HTTP-->>Service: status
Service-->>Controller: boolean
Controller-->>Client: 200 OK
end
rect rgba(240,230,255,0.4)
Client->>Controller: POST /transactions/signed
Controller->>Service: sendSignedTransaction(dto)
Service->>HTTP: POST /transactions/signed
HTTP->>API: Submit signed XDR
API-->>HTTP: tx hash or error
HTTP-->>Service: response
Service-->>Controller: result
Controller-->>Client: 200 OK / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (17)
package.json (1)
40-41: Centralize HTTP defaults — add keep‑alive agent and global retry/backoff interceptor.HttpModule is already registered at src/modules/trustlessWork/trustless-work.module.ts (timeout: 10000, maxRedirects: 5) and there are no direct axios.* calls in src; add an http(s).Agent with keepAlive and implement retries/backoff in a global Http interceptor so external calls use centralized, sane defaults instead of per‑call axios logic.
src/app.module.ts (2)
43-46: Validate env at bootAdd a validation schema so missing/mistyped TRUSTLESS_WORK_* fails fast.
Apply this diff:
- ConfigModule.forRoot({ - isGlobal: true, - envFilePath: ['.env'], - }), + ConfigModule.forRoot({ + isGlobal: true, + envFilePath: ['.env'], + cache: true, + validationSchema: Joi.object({ + DATABASE_URL: Joi.string().uri().required(), + TRUSTLESS_WORK_BASE_URL: Joi.string().uri().required(), + TRUSTLESS_WORK_ENV: Joi.string().valid('development', 'testnet', 'production').default('development'), + }), + }),Add this import at the top of the file:
import * as Joi from 'joi';
91-91: NitClass body spacing/style differs from the rest of the codebase; ignore if formatter enforces it.
src/modules/trustlessWork/trustless-work.module.ts (1)
9-14: Bind HttpModule to runtime configUse registerAsync and set Axios baseURL from TRUSTLESS_WORK_BASE_URL; also declare default headers and validateStatus.
Apply this diff:
- HttpModule.register({ - timeout: 10000, - maxRedirects: 5, - }), + HttpModule.registerAsync({ + inject: [ConfigService], + useFactory: (config: ConfigService) => ({ + baseURL: config.get<string>('TRUSTLESS_WORK_BASE_URL'), + timeout: 10_000, + maxRedirects: 5, + headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, + validateStatus: (s: number) => s >= 200 && s < 300, + }), + }),Note: ConfigService is already available via ConfigModule import.
src/modules/trustlessWork/tests/trustless-work-initialize.test.ts (5)
1-5: Fix imports and remove unused symbolNetworkType is unused; import EscrowType for type-safe usage below.
Apply this diff:
-import { NetworkType } from '../dtos/trustless-work.dto'; +import { EscrowType } from '../dtos/trustless-work.dto';
24-37: Avoidas any; use enumUse EscrowType.SINGLE_RELEASE for correctness and type safety.
Apply this diff:
- type: 'single-release' as any, + type: EscrowType.SINGLE_RELEASE,
49-72: Same here for multi-releaseUse enum instead of string+any.
Apply this diff:
- type: 'multi-release' as any, + type: EscrowType.MULTI_RELEASE,
44-46: Improve error logging for Axios errorsAccess response data when available for better diagnostics.
Apply this diff:
- } catch (error) { - console.log(' ❌ Single Release Failed:', error.message); + } catch (error: any) { + const msg = error?.response?.data ?? error?.message ?? error; + console.log(' ❌ Single Release Failed:', msg); }Repeat similarly for other catches.
Also applies to: 78-81, 98-100
29-31: Gitleaks false positive: public Stellar addresses — use env vars or allowlistThese are public Stellar account IDs (start with "G", not secret "S" keys). Replace hardcoded values in tests/docs with env vars or a central test fixture to avoid gitleaks noise and accidental real-key commits, or add a gitleaks allowlist for test/example account patterns.
Affected files: src/modules/trustlessWork/tests/trustless-work-initialize.test.ts (lines 29–32, 53–56, 89–91); also occurs in src/modules/auth/tests/auth.service.spec.ts, test/auth.e2e-spec.ts, openapi.yaml, README.md.
Apply this diff example:
- seller_key: 'GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ', + seller_key: process.env.TW_SELLER_KEY ?? 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA',Add a gitleaks allowlist for known-safe/test patterns if needed.
src/modules/trustlessWork/tests/trustless-work.service.ts (1)
52-68: Fix DTO/test mismatch: removenetworkfrom the test or addnetworkto DeploySingleReleaseEscrowDtoDeploySingleReleaseEscrowDto (src/modules/trustlessWork/dtos/trustless-work.dto.ts, ~lines 128–175) does not declare
network, while the test (src/modules/trustlessWork/tests/trustless-work.service.ts, lines 52–68) passesnetwork: 'testnet'. Either removenetworkfrom the test and type the variable as DeploySingleReleaseEscrowDto, or addnetwork: NetworkTypeto the DTO and propagate it through the service.src/modules/trustlessWork/controllers/trustless-work.controller.ts (1)
155-167: Consider GET + @query for read-only “details” endpoint (optional REST polish).Not blocking, but
GET /escrow/:type/details?contract_id=…&network=…improves semantics and cacheability.src/modules/trustlessWork/dtos/trustless-work.dto.ts (3)
200-203: Validate asset code shape (1–12 alphanumeric, uppercase).Stellar asset codes must be 1–12 alphanumeric chars.
@ApiProperty({ description: 'Asset code (e.g., USDC, XLM)' }) @IsString() + @Matches(/^[A-Z0-9]{1,12}$/, { message: 'Invalid asset code (A–Z, 0–9, 1–12 chars)' }) asset: string;Apply to both deploy DTOs.
Also applies to: 349-351
261-263: Harden numeric fields.Ensure integers and sane ranges.
- @IsNumber() + @IsInt() + @Min(0) milestone_index: number; @@ - @IsOptional() - @IsNumber() + @IsOptional() + @IsInt() + @Min(1) page?: number; @@ - @IsOptional() - @IsNumber() + @IsOptional() + @IsInt() + @Min(1) + @Max(100) limit?: number;Also applies to: 285-287, 377-385, 401-409
397-399: Make “role” an enum.Avoid arbitrary strings; define and enforce allowed roles.
export enum EscrowType { SINGLE_RELEASE = 'single-release', MULTI_RELEASE = 'multi-release', } + +export enum EscrowRole { + SELLER = 'seller', + APPROVER = 'approver', + RECEIVER = 'receiver', + DISPUTE_RESOLVER = 'dispute_resolver', +} @@ - @ApiProperty({ description: 'Role to filter by' }) - @IsString() - role: string; + @ApiProperty({ enum: EscrowRole, description: 'Role to filter by' }) + @IsEnum(EscrowRole) + role: EscrowRole;Also applies to: 5-13
src/modules/trustlessWork/services/trustless-work.service.ts (3)
337-343: Remove dead branch inreleaseFunds(endpoint name is the same).Simplify unnecessary conditional.
- const endpoint = escrowType === EscrowType.SINGLE_RELEASE ? 'release-funds' : 'release-funds'; const response = await firstValueFrom( - this.httpService.post(`${this.baseUrl}/escrow/${escrowType}/${endpoint}`, releaseDto, { + this.httpService.post(`${this.baseUrl}/escrow/${escrowType}/release-funds`, releaseDto, { headers: this.apiHeaders, - }) + timeout: this.defaultTimeoutMs, + }) );
62-67: Logger metadata usage is off; pass strings or stringify objects.
Logger.log(message, context?)expects a string context; objects become[object Object]. Errors should passerror?.stack.- this.logger.log('Initializing escrow', { - type: initDto.type, - seller: initDto.seller_key, - network: this.configService.get<string>('STELLAR_NETWORK') || 'testnet', - }); + this.logger.log( + `Initializing escrow ${initDto.type} seller=${initDto.seller_key} net=${this.configService.get<string>('STELLAR_NETWORK') || 'testnet'}` + ); @@ - this.logger.error('Failed to initialize escrow', error); + this.logger.error('Failed to initialize escrow', error?.stack); @@ - this.logger.log('Transaction sent successfully', { - transactionHash: response.data.transaction_hash, - contractId: transactionDto.contract_id, - }); + this.logger.log(`Transaction sent successfully tx=${response.data.transaction_hash} contract=${transactionDto.contract_id}`); @@ - this.logger.error('Failed to send transaction', error); + this.logger.error('Failed to send transaction', error?.stack);Replicate this pattern throughout (log/warn/error).
Also applies to: 121-124, 152-156, 182-189
665-680: Health check logging: pass stack/message correctly.Second arg to
warnis context, not the message. Useerror?.message.- } catch (error) { - this.logger.warn('Trustless Work API health check failed', error.message); + } catch (error) { + this.logger.warn(`Trustless Work API health check failed: ${error?.message}`); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.env.example(1 hunks)package.json(3 hunks)src/app.module.ts(2 hunks)src/modules/trustlessWork/controllers/trustless-work.controller.ts(1 hunks)src/modules/trustlessWork/dtos/trustless-work.dto.ts(1 hunks)src/modules/trustlessWork/services/trustless-work.service.ts(1 hunks)src/modules/trustlessWork/tests/trustless-work-initialize.test.ts(1 hunks)src/modules/trustlessWork/tests/trustless-work.service.ts(1 hunks)src/modules/trustlessWork/trustless-work.module.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/app.module.ts (1)
src/modules/trustlessWork/trustless-work.module.ts (1)
Module(7-19)
src/modules/trustlessWork/trustless-work.module.ts (1)
src/app.module.ts (1)
Module(41-91)
src/modules/trustlessWork/controllers/trustless-work.controller.ts (1)
src/modules/trustlessWork/dtos/trustless-work.dto.ts (15)
InitializeEscrowDto(38-108)SendSignedTransactionDto(110-124)DeploySingleReleaseEscrowDto(128-175)DeployMultiReleaseEscrowDto(177-226)FundEscrowDto(228-240)GetEscrowDto(242-250)ApproveMilestoneDto(252-264)ReleaseFundsDto(266-274)ReleaseMilestoneFundsDto(276-288)DisputeEscrowDto(290-302)ResolveDisputeDto(304-321)UpdateEscrowDto(323-341)SetTrustlineDto(343-360)SendTransactionDto(362-366)GetEscrowsByRoleDto(388-410)
src/modules/trustlessWork/tests/trustless-work.service.ts (1)
src/modules/trustlessWork/dtos/trustless-work.dto.ts (3)
DeploySingleReleaseEscrowDto(128-175)FundEscrowDto(228-240)ApproveMilestoneDto(252-264)
src/modules/trustlessWork/services/trustless-work.service.ts (1)
src/modules/trustlessWork/dtos/trustless-work.dto.ts (16)
InitializeEscrowDto(38-108)SendSignedTransactionDto(110-124)DeploySingleReleaseEscrowDto(128-175)DeployMultiReleaseEscrowDto(177-226)FundEscrowDto(228-240)GetEscrowDto(242-250)ApproveMilestoneDto(252-264)ReleaseFundsDto(266-274)ReleaseMilestoneFundsDto(276-288)DisputeEscrowDto(290-302)ResolveDisputeDto(304-321)UpdateEscrowDto(323-341)SetTrustlineDto(343-360)SendTransactionDto(362-366)GetEscrowsBySignerDto(368-386)GetEscrowsByRoleDto(388-410)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 20-20: [DuplicatedKey] The TRUSTLESS_WORK_ENV key is duplicated
(DuplicatedKey)
[warning] 21-21: [UnorderedKey] The TRUSTLESS_WORK_BASE_URL key should go before the TRUSTLESS_WORK_ENV key
(UnorderedKey)
🪛 Gitleaks (8.28.0)
src/modules/trustlessWork/tests/trustless-work-initialize.test.ts
[high] 29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
package.json (1)
74-74: stellar-sdk already pinned; address soroban-client deprecation.
- package.json has "stellar-sdk": "11.3.0" — pinning concern resolved.
- package-lock.json shows deprecation notices: soroban-client is deprecated (migration → @stellar/stellar-sdk) and stellar-sdk moved to @stellar/stellar-sdk.
- Check imports/usages: test/auth.e2e-spec.ts, src/modules/auth/tests/auth.service.spec.ts, src/modules/auth/services/auth.service.ts.
Action: migrate soroban-client → @stellar/stellar-sdk per the migration guide or add npm overrides/lock exact versions; run full test/e2e suite and npm audit.
src/app.module.ts (2)
39-39: Module wiring LGTMImporting TrustlessWorkModule is correct.
88-88: Including TrustlessWorkModule in imports is correctNo issues spotted.
src/modules/trustlessWork/tests/trustless-work-initialize.test.ts (1)
18-22: Consider awaiting config or make method syncIf getApiConfiguration is async, add await; otherwise fine. Confirm actual return type.
src/modules/trustlessWork/tests/trustless-work.service.ts (5)
88-110: Fund flow test LGTMEndpoint and expectations look consistent.
112-135: Get escrow details LGTMCovers happy path.
137-159: Approve milestone LGTMGood coverage for MULTI_RELEASE path.
161-183: Release funds LGTMMatches SINGLE_RELEASE path and asserts endpoint.
185-208: Dispute flow LGTMLooks good.
| TRUSTLESS_WORK_ENV=development | ||
|
|
||
| SOROBAN_RPC_URL=https://your-soroban-endpoint | ||
| SOROBAN_SERVER_SECRET=your-secret-key | ||
|
|
||
| TRUSTLESS_WORK_ENV=testnet | ||
| TRUSTLESS_WORK_BASE_URL=https://api.trustlesswork.com |
There was a problem hiding this comment.
Deduplicate and order TRUSTLESS_WORK_ keys*
You added TRUSTLESS_WORK_ENV twice and placed TRUSTLESS_WORK_BASE_URL after it. This can mislead users and break dotenv linters.
Apply this diff:
-TRUSTLESS_WORK_ENV=development
+TRUSTLESS_WORK_BASE_URL=https://api.trustlesswork.com
+TRUSTLESS_WORK_ENV=development
...
-TRUSTLESS_WORK_ENV=testnet
-TRUSTLESS_WORK_BASE_URL=https://api.trustlesswork.comOptional: document valid values for TRUSTLESS_WORK_ENV (development|testnet|production).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TRUSTLESS_WORK_ENV=development | |
| SOROBAN_RPC_URL=https://your-soroban-endpoint | |
| SOROBAN_SERVER_SECRET=your-secret-key | |
| TRUSTLESS_WORK_ENV=testnet | |
| TRUSTLESS_WORK_BASE_URL=https://api.trustlesswork.com | |
| TRUSTLESS_WORK_BASE_URL=https://api.trustlesswork.com | |
| TRUSTLESS_WORK_ENV=development | |
| SOROBAN_RPC_URL=https://your-soroban-endpoint | |
| SOROBAN_SERVER_SECRET=your-secret-key |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 20-20: [DuplicatedKey] The TRUSTLESS_WORK_ENV key is duplicated
(DuplicatedKey)
[warning] 21-21: [UnorderedKey] The TRUSTLESS_WORK_BASE_URL key should go before the TRUSTLESS_WORK_ENV key
(UnorderedKey)
🤖 Prompt for AI Agents
In .env.example around lines 15 to 21, TRUSTLESS_WORK_ENV is declared twice and
TRUSTLESS_WORK_BASE_URL is placed after the duplicate which can confuse users
and break dotenv linters; remove the duplicate TRUSTLESS_WORK_ENV so it appears
only once, place TRUSTLESS_WORK_BASE_URL immediately next to the
TRUSTLESS_WORK_* group, and add a short comment documenting valid values for
TRUSTLESS_WORK_ENV (development|testnet|production).
| "class-transformer": "^0.5.1", | ||
| "class-validator": "^0.14.2", | ||
| "cloudinary": "^1.41.3", | ||
| "crypto": "^1.0.1", |
There was a problem hiding this comment.
🧩 Analysis chain
Remove npm "crypto" dependency; use built‑in Node crypto.
The npm package crypto@^1.0.1 is unnecessary and risks shadowing the Node core module. Drop it and import from node:crypto.
Apply this diff:
"class-validator": "^0.14.2",
"cloudinary": "^1.41.3",
- "crypto": "^1.0.1",
"dotenv": "^16.4.7",Outside this file, ensure imports use the core module:
import { createHmac } from 'node:crypto';Find and fix any non‑core imports:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C1 "from ['\"]crypto['\"]|require\(['\"]crypto['\"]\)"Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "package.json: matching '\"crypto\":'"
rg -n '^\s*"crypto"\s*:' package.json || true
echo
echo "Search repo for imports/requires of 'crypto' or 'node:crypto':"
rg -nP -C1 "(from\s+['\"](crypto|node:crypto)['\"])|(require\(['\"](crypto|node:crypto)['\"]\))" || true
echo
echo "Show src/config/crypto-global.ts (if present):"
sed -n '1,160p' src/config/crypto-global.ts || trueLength of output: 1211
Remove npm "crypto" dependency; import from Node core 'node:crypto'.
Delete the npm "crypto" entry from package.json and update imports to the Node core specifier to avoid shadowing the built-in module.
"class-validator": "^0.14.2",
"cloudinary": "^1.41.3",
- "crypto": "^1.0.1",
"dotenv": "^16.4.7",Change in src/config/crypto-global.ts:
- replace: import * as crypto from 'crypto';
- with: import * as crypto from 'node:crypto';
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "crypto": "^1.0.1", | |
| "class-validator": "^0.14.2", | |
| "cloudinary": "^1.41.3", | |
| "dotenv": "^16.4.7", |
| "crypto": "^1.0.1", | |
| import * as crypto from 'node:crypto'; |
🤖 Prompt for AI Agents
In package.json around line 57 remove the "crypto": "^1.0.1" dependency entry;
then update code imports that reference the npm package to use the Node core
specifier (e.g., in src/config/crypto-global.ts replace imports from 'crypto'
with 'node:crypto'); after removing the dependency run npm install (or yarn) to
update package-lock.json/yarn.lock and search the repo to replace any remaining
"crypto" imports with "node:crypto" to avoid shadowing the built-in module.
| @Post('send-transaction') | ||
| @HttpCode(HttpStatus.OK) | ||
| @ApiOperation({ | ||
| summary: 'Send signed transaction to Stellar network', | ||
| description: 'Submits a signed XDR transaction to complete escrow creation', | ||
| }) | ||
| @ApiResponse({ | ||
| status: 200, | ||
| description: 'Transaction sent successfully', | ||
| schema: { | ||
| example: { | ||
| success: true, | ||
| transaction_hash: 'abc123...', | ||
| message: 'Transaction sent successfully', | ||
| }, | ||
| }, | ||
| }) | ||
| @ApiResponse({ | ||
| status: 400, | ||
| description: 'Missing signature or invalid XDR', | ||
| }) | ||
| async sendSignedTransaction(@Body() transactionDto: SendSignedTransactionDto) { | ||
| return this.trustlessWorkService.sendSignedTransaction(transactionDto); | ||
| } |
There was a problem hiding this comment.
Two “send transaction” endpoints with mismatched DTOs; unify semantics.
sendSignedTransaction requires network, while helper/send-transaction does not. This is confusing and likely inconsistent with the upstream API.
- Keep a single endpoint (e.g.,
POST /send-transaction) requiringnetworkandsigned_xdr. - Remove or deprecate
helper/send-transaction, or extendSendTransactionDtoto includenetworkand route both to the same service method.
Also applies to: 249-255
🤖 Prompt for AI Agents
In src/modules/trustlessWork/controllers/trustless-work.controller.ts around
lines 67-90 (and similarly lines 249-255), there are two "send transaction"
endpoints with mismatched DTOs: one requires network while the other does not;
unify them so the API semantics are consistent. Update the controller to expose
a single POST /send-transaction endpoint that accepts a unified DTO containing
both network and signed_xdr (or extend SendTransactionDto to include network)
and route both callers to the same service method; remove or deprecate the
helper/send-transaction endpoint or make it delegate to this unified route to
avoid duplication.
| async getSellerEscrows( | ||
| @Param('sellerKey') sellerKey: string, | ||
| @Body() queryDto: Omit<GetEscrowsBySignerDto, 'signer'>, | ||
| ) { | ||
| return this.trustlessWorkService.getEscrowsBySigner({ | ||
| ...queryDto, | ||
| signer: sellerKey, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Omit<...> in handler disables class‑validator; define a concrete DTO to keep validation.
Omit<GetEscrowsBySignerDto, 'signer'> erases runtime metadata, so ValidationPipe won’t validate body. Create a dedicated DTO (e.g., GetSellerEscrowsDto) and use it here.
- async getSellerEscrows(
- @Param('sellerKey') sellerKey: string,
- @Body() queryDto: Omit<GetEscrowsBySignerDto, 'signer'>,
- ) {
+ async getSellerEscrows(
+ @Param('sellerKey') sellerKey: string,
+ @Body() queryDto: GetSellerEscrowsDto,
+ ) {
return this.trustlessWorkService.getEscrowsBySigner({
...queryDto,
signer: sellerKey,
});
}Add GetSellerEscrowsDto to dtos/trustless-work.dto.ts with network, optional page, limit (see DTO comment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getSellerEscrows( | |
| @Param('sellerKey') sellerKey: string, | |
| @Body() queryDto: Omit<GetEscrowsBySignerDto, 'signer'>, | |
| ) { | |
| return this.trustlessWorkService.getEscrowsBySigner({ | |
| ...queryDto, | |
| signer: sellerKey, | |
| }); | |
| } | |
| async getSellerEscrows( | |
| @Param('sellerKey') sellerKey: string, | |
| @Body() queryDto: GetSellerEscrowsDto, | |
| ) { | |
| return this.trustlessWorkService.getEscrowsBySigner({ | |
| ...queryDto, | |
| signer: sellerKey, | |
| }); | |
| } |
| @Get('health') | ||
| @ApiOperation({ summary: 'Check Trustless Work API health' }) | ||
| async healthCheck() { | ||
| const isHealthy = await this.trustlessWorkService.healthCheck(); | ||
| const config = this.trustlessWorkService.getApiConfiguration(); | ||
|
|
||
| return { | ||
| status: isHealthy ? 'healthy' : 'unhealthy', | ||
| timestamp: new Date().toISOString(), | ||
| configuration: config, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Health endpoint leaks config; trim response or gate in prod.
Returning baseUrl and hasApiKey increases attack surface.
- const config = this.trustlessWorkService.getApiConfiguration();
-
- return {
- status: isHealthy ? 'healthy' : 'unhealthy',
- timestamp: new Date().toISOString(),
- configuration: config,
- };
+ return {
+ status: isHealthy ? 'healthy' : 'unhealthy',
+ timestamp: new Date().toISOString(),
+ };Optionally add an env gate to include config only in non‑prod.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Get('health') | |
| @ApiOperation({ summary: 'Check Trustless Work API health' }) | |
| async healthCheck() { | |
| const isHealthy = await this.trustlessWorkService.healthCheck(); | |
| const config = this.trustlessWorkService.getApiConfiguration(); | |
| return { | |
| status: isHealthy ? 'healthy' : 'unhealthy', | |
| timestamp: new Date().toISOString(), | |
| configuration: config, | |
| }; | |
| } | |
| @Get('health') | |
| @ApiOperation({ summary: 'Check Trustless Work API health' }) | |
| async healthCheck() { | |
| const isHealthy = await this.trustlessWorkService.healthCheck(); | |
| return { | |
| status: isHealthy ? 'healthy' : 'unhealthy', | |
| timestamp: new Date().toISOString(), | |
| }; | |
| } |
| const response = await firstValueFrom( | ||
| this.httpService.post(`${this.baseUrl}/helper/send-transaction`, { | ||
| network: transactionDto.network, | ||
| signed_xdr: transactionDto.signed_xdr, | ||
| }, { | ||
| headers: this.apiHeaders, | ||
| }) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add client timeouts for all external calls (avoid request thread hangs).
Most Axios calls have no timeout (except health). This is a production risk.
+ private readonly defaultTimeoutMs: number;
@@
constructor(
private readonly httpService: HttpService,
private readonly configService: ConfigService,
) {
@@
+ this.defaultTimeoutMs = Number(this.configService.get('TRUSTLESS_WORK_TIMEOUT_MS')) || 10000;
@@
- }, {
- headers: this.apiHeaders,
- })
+ }, { headers: this.apiHeaders, timeout: this.defaultTimeoutMs })
@@
- }, {
- headers: this.apiHeaders,
- })
+ }, { headers: this.apiHeaders, timeout: this.defaultTimeoutMs })Apply { timeout: this.defaultTimeoutMs } to all HTTP calls in this service (deploy, fund, details, milestone, disputes, helpers, lists).
Also applies to: 176-180, 196-200, 221-224, 246-249, 266-272, 294-297, 340-343, 364-367, 393-396, 417-420, 437-440, 454-457, 478-481, 506-509, 526-529, 547-550, 569-572
🤖 Prompt for AI Agents
In src/modules/trustlessWork/services/trustless-work.service.ts around lines
143-151 (and also at the other listed ranges: 176-180, 196-200, 221-224,
246-249, 266-272, 294-297, 340-343, 364-367, 393-396, 417-420, 437-440, 454-457,
478-481, 506-509, 526-529, 547-550, 569-572), the axios HTTP calls are missing
per-request timeouts; update each this.httpService.post/get/patch call to
include the request option { timeout: this.defaultTimeoutMs } (i.e., merge into
the existing third-argument config object or create one if absent) so every
external call in this service uses this.defaultTimeoutMs to avoid hanging
requests.
| async changeMilestoneStatus(escrowType: EscrowType, statusDto: any): Promise<any> { | ||
| this.logger.log(`Changing milestone status for ${escrowType} escrow`); | ||
| try { | ||
| const response = await firstValueFrom( | ||
| this.httpService.post(`${this.baseUrl}/escrow/${escrowType}/change-milestone-status`, statusDto, { | ||
| headers: this.apiHeaders, | ||
| }) | ||
| ); | ||
|
|
||
| this.logger.log('Milestone status changed successfully'); | ||
| return response.data; | ||
| } catch (error) { | ||
| this.logger.error('Failed to change milestone status', error); | ||
| this.handleApiError(error, 'Change Milestone Status'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid any in dispute/milestone helpers; define DTOs.
changeMilestoneStatus, disputeMilestone, resolveMilestoneDispute accept any. Add explicit DTOs to enforce payload shape.
I can draft ChangeMilestoneStatusDto, DisputeMilestoneDto, ResolveMilestoneDisputeDto mirroring upstream API.
Also applies to: 433-465
| async sendTransaction(transactionDto: SendTransactionDto): Promise<any> { | ||
| this.logger.log('Sending transaction to Stellar network', { | ||
| network: this.configService.get<string>('STELLAR_NETWORK') || 'testnet', | ||
| }); | ||
|
|
||
| try { | ||
| const response = await firstValueFrom( | ||
| this.httpService.post(`${this.baseUrl}/helper/send-transaction`, transactionDto, { | ||
| headers: this.apiHeaders, | ||
| }) | ||
| ); | ||
|
|
||
| this.logger.log('Transaction sent successfully'); | ||
| return response.data; | ||
| } catch (error) { | ||
| this.logger.error('Failed to send transaction', error); | ||
| this.handleApiError(error, 'Send Transaction'); | ||
| } | ||
| } |
There was a problem hiding this comment.
sendTransaction omits network in payload; align with sendSignedTransaction.
Upstream endpoint likely expects network. Include configured network or accept it from DTO.
async sendTransaction(transactionDto: SendTransactionDto): Promise<any> {
this.logger.log('Sending transaction to Stellar network', {
network: this.configService.get<string>('STELLAR_NETWORK') || 'testnet',
});
@@
- const response = await firstValueFrom(
- this.httpService.post(`${this.baseUrl}/helper/send-transaction`, transactionDto, {
- headers: this.apiHeaders,
- })
- );
+ const response = await firstValueFrom(
+ this.httpService.post(
+ `${this.baseUrl}/helper/send-transaction`,
+ {
+ network:
+ transactionDto.network ??
+ (this.configService.get<string>('STELLAR_NETWORK') || 'testnet'),
+ signed_xdr: transactionDto.signed_xdr,
+ },
+ { headers: this.apiHeaders, timeout: this.defaultTimeoutMs },
+ )
+ );Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/trustlessWork/services/trustless-work.service.ts around lines 519
to 537, sendTransaction currently posts transactionDto without including the
network field which the upstream helper/send-transaction endpoint expects;
modify the payload to include a network property by reading the network from
transactionDto if present or falling back to
this.configService.get<string>('STELLAR_NETWORK') || 'testnet', and send that
augmented object to the HTTP post; also ensure any typing reflects the optional
network field so the method compiles and tests still pass.
| private handleApiError(error: any, operation: string): never { | ||
| const status = error.response?.status || HttpStatus.INTERNAL_SERVER_ERROR; | ||
| const message = error.response?.data?.message || error.message || `Trustless Work API Error in ${operation}`; | ||
| const details = error.response?.data || {}; | ||
|
|
||
| this.logger.error(`API Error in ${operation}`, { | ||
| status, | ||
| message, | ||
| details, | ||
| url: error.config?.url, | ||
| }); | ||
|
|
||
| // Mapear códigos de estado específicos | ||
| let mappedStatus = status; | ||
| if (status === 429) { | ||
| mappedStatus = HttpStatus.TOO_MANY_REQUESTS; | ||
| } else if (status >= 400 && status < 500) { | ||
| mappedStatus = HttpStatus.BAD_REQUEST; | ||
| } else if (status >= 500) { | ||
| mappedStatus = HttpStatus.SERVICE_UNAVAILABLE; | ||
| } | ||
|
|
||
| throw new HttpException( | ||
| { | ||
| statusCode: mappedStatus, | ||
| message, | ||
| error: `Trustless Work API Error - ${operation}`, | ||
| details, | ||
| timestamp: new Date().toISOString(), | ||
| }, | ||
| mappedStatus, | ||
| ); |
There was a problem hiding this comment.
Preserve upstream status codes and sanitize error bodies.
Coalescing all 4xx to 400 hides 401/403/404. Also, echoing details to clients can leak internals.
- const status = error.response?.status || HttpStatus.INTERNAL_SERVER_ERROR;
- const message = error.response?.data?.message || error.message || `Trustless Work API Error in ${operation}`;
- const details = error.response?.data || {};
+ const status = error.response?.status ?? HttpStatus.INTERNAL_SERVER_ERROR;
+ const message =
+ error.response?.data?.message ??
+ error.message ??
+ `Trustless Work API Error in ${operation}`;
+ // Avoid leaking full upstream payloads; keep a small, safe subset
+ const details =
+ typeof error.response?.data === 'object'
+ ? { code: error.response.data.code, error: error.response.data.error }
+ : {};
@@
- let mappedStatus = status;
- if (status === 429) {
- mappedStatus = HttpStatus.TOO_MANY_REQUESTS;
- } else if (status >= 400 && status < 500) {
- mappedStatus = HttpStatus.BAD_REQUEST;
- } else if (status >= 500) {
- mappedStatus = HttpStatus.SERVICE_UNAVAILABLE;
- }
+ const mappedStatus =
+ status === 429
+ ? HttpStatus.TOO_MANY_REQUESTS
+ : status >= 500
+ ? HttpStatus.SERVICE_UNAVAILABLE
+ : status;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleApiError(error: any, operation: string): never { | |
| const status = error.response?.status || HttpStatus.INTERNAL_SERVER_ERROR; | |
| const message = error.response?.data?.message || error.message || `Trustless Work API Error in ${operation}`; | |
| const details = error.response?.data || {}; | |
| this.logger.error(`API Error in ${operation}`, { | |
| status, | |
| message, | |
| details, | |
| url: error.config?.url, | |
| }); | |
| // Mapear códigos de estado específicos | |
| let mappedStatus = status; | |
| if (status === 429) { | |
| mappedStatus = HttpStatus.TOO_MANY_REQUESTS; | |
| } else if (status >= 400 && status < 500) { | |
| mappedStatus = HttpStatus.BAD_REQUEST; | |
| } else if (status >= 500) { | |
| mappedStatus = HttpStatus.SERVICE_UNAVAILABLE; | |
| } | |
| throw new HttpException( | |
| { | |
| statusCode: mappedStatus, | |
| message, | |
| error: `Trustless Work API Error - ${operation}`, | |
| details, | |
| timestamp: new Date().toISOString(), | |
| }, | |
| mappedStatus, | |
| ); | |
| private handleApiError(error: any, operation: string): never { | |
| const status = error.response?.status ?? HttpStatus.INTERNAL_SERVER_ERROR; | |
| const message = | |
| error.response?.data?.message ?? | |
| error.message ?? | |
| `Trustless Work API Error in ${operation}`; | |
| // Avoid leaking full upstream payloads; keep a small, safe subset | |
| const details = | |
| error.response?.data != null && typeof error.response.data === 'object' | |
| ? { code: error.response.data.code, error: error.response.data.error } | |
| : {}; | |
| this.logger.error(`API Error in ${operation}`, { | |
| status, | |
| message, | |
| details, | |
| url: error.config?.url, | |
| }); | |
| // Mapear códigos de estado específicos | |
| const mappedStatus = | |
| status === 429 | |
| ? HttpStatus.TOO_MANY_REQUESTS | |
| : status >= 500 | |
| ? HttpStatus.SERVICE_UNAVAILABLE | |
| : status; | |
| throw new HttpException( | |
| { | |
| statusCode: mappedStatus, | |
| message, | |
| error: `Trustless Work API Error - ${operation}`, | |
| details, | |
| timestamp: new Date().toISOString(), | |
| }, | |
| mappedStatus, | |
| ); |
🤖 Prompt for AI Agents
In src/modules/trustlessWork/services/trustless-work.service.ts around lines
698-729, the handler currently collapses all 4xx statuses to 400 and returns the
raw error.response.data to clients; change it to preserve upstream 4xx (so
401/403/404 remain unchanged) while still mapping 429 to
HttpStatus.TOO_MANY_REQUESTS and mapping server-side (>=500) errors to
HttpStatus.SERVICE_UNAVAILABLE. Keep logging the full error.details internally,
but sanitize the object sent in the HttpException by whitelisting only safe
fields (e.g., message, code, maybe a generic reason) or redacting any risky
fields (stack, headers, internal IDs, raw SQL), and include timestamp,
statusCode and a generic error string; then throw HttpException using the
preserved/mapped status and the sanitized payload.
| // Ejecutar solo si se llama directamente | ||
| if (require.main === module) { | ||
| testTrustlessWork().catch(console.error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Prevent accidental CI execution
This is a manual harness, not a Jest test. Gate execution by env var or move to scripts/.
Apply this diff:
-if (require.main === module) {
- testTrustlessWork().catch(console.error);
-}
+if (require.main === module && process.env.RUN_TW_E2E === 'true') {
+ testTrustlessWork().catch(console.error);
+}Run to confirm Jest doesn’t pick it up:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 'describe\s*\(' src/modules/trustlessWork/tests/trustless-work-initialize.test.ts || echo "No Jest suites found (good)"Length of output: 27
Gate manual harness to avoid accidental CI execution
Manual harness (not a Jest suite). Verified: no 'describe' suites found in src/modules/trustlessWork/tests/trustless-work-initialize.test.ts. Gate execution by env var or move to scripts/.
Apply this diff:
-if (require.main === module) {
- testTrustlessWork().catch(console.error);
-}
+if (require.main === module && process.env.RUN_TW_E2E === 'true') {
+ testTrustlessWork().catch(console.error);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ejecutar solo si se llama directamente | |
| if (require.main === module) { | |
| testTrustlessWork().catch(console.error); | |
| } | |
| // Ejecutar solo si se llama directamente | |
| if (require.main === module && process.env.RUN_TW_E2E === 'true') { | |
| testTrustlessWork().catch(console.error); | |
| } |
🤖 Prompt for AI Agents
In src/modules/trustlessWork/tests/trustless-work-initialize.test.ts around
lines 111 to 114, the file contains a manual harness that runs
testTrustlessWork() when executed directly which can be accidentally invoked by
CI; gate this manual execution behind an explicit environment variable (e.g., if
(require.main === module && process.env.RUN_MANUAL_TRUSTLESS_TEST === '1') {
testTrustlessWork().catch(console.error); }) or alternatively remove the manual
block from the test file and move it into a separate script under scripts/ that
is invoked manually; update any README or npm scripts if you add the env var
approach so maintainers know how to run it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app.module.ts (1)
54-57: The explicitenvFilePathis redundant.NestJS's
ConfigModuleloads.envfrom the project root by default whenenvFilePathis not specified. This explicit configuration is unnecessary unless you plan to support multiple environment files or non-standard paths.If you want to maintain the default behavior, you can simplify this to:
- ConfigModule.forRoot({ - isGlobal: true, - envFilePath: ['.env'], - }), + ConfigModule.forRoot({ isGlobal: true }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(3 hunks)src/app.module.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit inference engine (.cursorrules)
src/**/*.ts: Do not access environment variables via process.env directly; import and use the config object from src/config/env
Provide explicit return types for all functions
Do not use the any type; use unknown or specific types instead
Create interfaces for complex object shapes
Type all function parameters explicitly
Do not use console.log; use proper logging (e.g., NestJS Logger)
Remove unused imports
Do not leave commented-out code in commits
Wrap risky operations in try-catch and handle errors appropriately
Add JSDoc comments for complex logic
Files:
src/app.module.ts
🧬 Code graph analysis (1)
src/app.module.ts (2)
src/modules/escrows/escrows.module.ts (1)
Module(8-14)src/modules/trustlessWork/trustless-work.module.ts (1)
Module(7-19)
🔇 Additional comments (3)
src/app.module.ts (3)
23-24: LGTM!The module imports are correctly formatted and the paths match the module definitions provided in the context.
106-110: Clarify dual registration of EscrowModule and EscrowsModule
We verified routes in src/modules/escrow and src/modules/escrows—no HTTP method/path collisions found. Confirm the intent and plan for both modules:
- Is this part of migrating from the legacy to the v2 escrow API?
- What is the deprecation timeline and transition strategy for the old EscrowModule?
- If both must coexist permanently, document each module’s use cases and responsibilities.
84-88: Verify V2 entity table mappings before dual registration. Confirm thatEscrowV2andMilestoneV2use distinct table names or aliases in their@Entity(...)decorators so that registering both original and V2 entities cannot cause schema-sync conflicts or runtime errors.
| import { Escrow as EscrowV2 } from './modules/escrows/entities/escrow.entity'; | ||
| import { Milestone as MilestoneV2 } from './modules/escrows/entities/milestone.entity'; |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the dual entity architecture.
Aliasing entities as EscrowV2 and MilestoneV2 while retaining the original Escrow and Milestone entities (lines 45-46) suggests a parallel system or incomplete migration. Both versions are registered in TypeORM (lines 83-84 and 87-88), which will cause table conflicts unless the entities explicitly use different table names via the @Entity decorator.
Run the following script to verify the table names configured for these entities:
If the entities target the same tables, this will cause schema conflicts. Consider:
- Using different table names if both systems must coexist
- Removing the old entities if migrating to the new architecture
- Documenting the migration strategy clearly
🏁 Script executed:
#!/bin/bash
# Description: Check @Entity decorator table names for all Escrow and Milestone entities
echo "=== Old Escrow entity ==="
ast-grep --pattern $'@Entity($$$)
export class Escrow $$$'
echo -e "\n=== New Escrow (EscrowV2) entity ==="
ast-grep --pattern $'@Entity($$$)
export class Escrow $$$' ./src/modules/escrows/entities/escrow.entity.ts
echo -e "\n=== Old Milestone entity ==="
ast-grep --pattern $'@Entity($$$)
export class Milestone $$$'
echo -e "\n=== New Milestone (MilestoneV2) entity ==="
ast-grep --pattern $'@Entity($$$)
export class Milestone $$$' ./src/modules/escrows/entities/milestone.entity.tsLength of output: 137
🏁 Script executed:
#!/bin/bash
echo "=== escrow.entity.ts files and @Entity decorators ==="
fd "escrow.entity.ts" -x sh -c 'echo "-- File: {}"; rg -n "@Entity" {} || echo "No @Entity found"'
echo -e "\n=== milestone.entity.ts files and @Entity decorators ==="
fd "milestone.entity.ts" -x sh -c 'echo "-- File: {}"; rg -n "@Entity" {} || echo "No @Entity found"'Length of output: 443
Escrow entities conflict on the same table
- Both
Escrow(./src/modules/escrow/entities/escrow.entity.ts) andEscrowV2(./src/modules/escrows/entities/escrow.entity.ts) use@Entity('escrows'), causing schema collisions. Rename one table or remove the legacy entity. Milestone('milestones') andMilestoneV2('escrow_milestones') map to different tables, so no collision there.
🤖 Prompt for AI Agents
In src/app.module.ts around lines 49-50, two different entity classes are both
mapped to the same database table name ('escrows') causing a schema collision;
remove or rename one mapping to resolve it. Identify which entity is legacy
(Escrow from ./modules/escrow) or the v2 (EscrowV2 from ./modules/escrows), then
either delete the legacy entity import and usages or change its @Entity()
decorator to a distinct table name (or rename V2 instead) so only one class maps
to 'escrows'. Update all imports/usages across the codebase and any TypeORM
config/migrations to reflect the chosen canonical entity, and run
tests/migrations to ensure no further schema conflicts.
1 participant
🚀 StarShop Pull Request
Issue #157
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Initialize escrow, use directly API, i don't use sdk to this implementation.
Dtos to validate information send to the front-end
⏰ Time spent breakdown
5 hours
🌌 Comments
Review multer in file.service.ts is important
Summary by CodeRabbit
New Features
Chores
Tests