Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds a Redis-backed caching subsystem and integrates it into products. Introduces extensive escrow functionality (accounts, milestones, funding, approvals/releases) plus a parallel “escrows” flow. Migrates user IDs to UUIDs across domains, shifts auth and services to walletAddress-centric logic, adds disputes and on-chain order metadata, expands user fields, adds seller Soroban registration, strengthens observability, and updates numerous migrations and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Ctrl as ProductsController
participant Svc as ProductService
participant Int as CacheInterceptor
participant CS as CacheService
participant Store as Redis Store
Client->>Ctrl: GET /products?page=...
Ctrl->>Int: handler invoked
Int->>CS: generateKey(entity=product, action=list, params)
CS->>Store: get(key)
alt Cache HIT
Store-->>CS: value
CS-->>Int: value
Int-->>Ctrl: cached response
Ctrl-->>Client: 200 JSON
else Cache MISS
Int->>Svc: getAll(...)
Svc-->>Int: data
Int->>CS: set(key, data, ttl)
CS->>Store: set(key, data, ttl)
Int-->>Ctrl: data
Ctrl-->>Client: 200 JSON
end
sequenceDiagram
autonumber
actor Buyer
actor Seller
participant EC as EscrowController
participant ES as EscrowService
participant DB as DB (Tx)
Buyer->>EC: POST /escrow/approve-milestone
EC->>ES: approveMilestone(dto, buyerId)
ES->>DB: Tx start: load milestone+escrow
ES->>DB: validate + update milestone(status=APPROVED)
DB-->>ES: saved milestone
ES-->>EC: { success, milestone }
EC-->>Buyer: 200
Seller->>EC: POST /escrow/release-funds
EC->>ES: releaseFunds(dto, sellerId)
ES->>DB: Tx start: validate seller + milestone state
ES->>DB: update milestone.releasedAt + escrow.releasedAmount
DB-->>ES: saved
ES-->>EC: { success, data: release summary }
EC-->>Seller: 200
sequenceDiagram
autonumber
actor Seller
participant SC as SellerController
participant SS as SellerService
Seller->>SC: POST /seller/contract/build-register {payoutWallet}
SC->>SS: buildRegister(userId, payoutWallet)
SS-->>SC: { unsignedXdr, contractAddress }
SC-->>Seller: 200 { success, data }
Seller->>SC: POST /seller/contract/submit {signedXdr}
SC->>SS: submitRegister(userId, signedXdr)
SS-->>SC: { transactionHash, contractId, payoutWallet, registered }
SC-->>Seller: 200 { success, data }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
✨ Finishing touches
🧪 Generate unit tests
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: 74
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
src/modules/files/tests/file.service.spec.ts (2)
178-194: ID type inconsistency detected.Line 179 uses
id: 1(number), but other mockUser instances in this file useid: "1"(string). This inconsistency should be resolved for test data uniformity.Apply this diff to use string IDs consistently:
const mockUser = { - id: 1, + id: '1', walletAddress: '0x123', name: 'Test User', email: 'test@example.com', location: 'Test City', country: 'Test Country', buyerData: {}, sellerData: null, orders: [], userRoles: [], notifications: [], wishlist: [], stores: [], createdAt: new Date(), updatedAt: new Date(), };
252-280: Remove commented-out test data.Lines 267-279 contain a large block of commented-out code (second file in the array). Per coding guidelines: "Do not leave commented-out code in commits."
Apply this diff to remove the commented code:
const mockFiles: File[] = [ { id: 'f1', uploadedById: '1', uploadedBy: mockUser, - url: 'https://example.com/test.jpg', type: FileType.IMAGE, filename: 'test.jpg', mimetype: 'image/jpeg', size: 1024, providerType: 'cloudinary', providerPublicId: 'public-id', uploadedAt: new Date(), }, - // { - // id: 'f2', - // uploadedById: '1', - // uploadedBy: mockUser, - // url: 'https://example.com/test.pdf', - // type: FileType.DOCUMENT, - // filename: 'test.pdf', - // mimetype: 'application/pdf', - // size: 2048, - // providerType: 's3', - // providerPublicId: 'public-id', - // uploadedAt: new Date(), - // }, ] as File[];src/modules/auth/strategies/jwt.strategy.ts (1)
47-49: Error handling swallows diagnostic context.The catch block re-throws a generic
UnauthorizedException('Invalid token')which discards the original error details. This makes debugging authentication failures difficult.Consider logging the original error before re-throwing, or conditionally preserve error messages in non-production environments:
} catch (error) { + this.logger.error('JWT validation failed', error); throw new UnauthorizedException('Invalid token'); }Note: If the generic message is intentional for security (to avoid leaking information to clients), ensure server-side logging captures the details.
src/modules/auth/middleware/jwt-auth.middleware.ts (1)
45-51: Remove theas anytype assertion.The
as anyassertion on line 51 directly violates the coding guideline: "Do not use the any type; use unknown or specific types instead." This bypasses type checking and creates maintenance risks.Define a proper interface for the user payload and use it instead:
+interface AuthenticatedUser { + id: string; + email: string; + walletAddress: string; + name: string; + role: Role[]; +} + export const jwtAuthMiddleware = async (req: Request, res: Response, next: NextFunction) => { // ... req.user = { id: user.id, email: user.email, walletAddress: user.walletAddress, name: user.name, role: user.userRoles?.map((ur) => ur.role.name as Role) || [decoded.role as Role], - } as any; + } as AuthenticatedUser;Alternatively, extend the Express Request type via declaration merging in a types file to eliminate the assertion entirely.
As per coding guidelines.
src/main.ts (2)
57-60: Replace console.log with proper logging.Per coding guidelines,
console.logandconsole.errorshould not be used. Use NestJS Logger instead for structured logging and proper log levels.Apply this diff:
+import { Logger } from '@nestjs/common'; + async function bootstrap(): Promise<void> { + const logger = new Logger('Bootstrap'); const app = await NestFactory.create(AppModule); // ... try { const schedulerService = app.get(BuyerRequestSchedulerService); const closedCount = await schedulerService.closeExpiredRequests(); - console.log(`Startup: Closed ${closedCount} expired buyer requests`); + logger.log(`Startup: Closed ${closedCount} expired buyer requests`); } catch (error) { - console.error('Failed to close expired requests on startup:', error); + logger.error('Failed to close expired requests on startup:', error); }
62-65: Use config object and Logger for environment variables and logging.Lines 62, 64-65 violate coding guidelines:
- Direct
process.env.PORTaccess (should use config object from src/config/env)- Multiple
console.logstatements (should use NestJS Logger)Apply this diff:
+import { Logger } from '@nestjs/common'; +import config from './config/env'; + async function bootstrap(): Promise<void> { + const logger = new Logger('Bootstrap'); const app = await NestFactory.create(AppModule); // ... - const port = process.env.PORT || 3000; + const port = config.port || 3000; await app.listen(port); - console.log(`Application is running on: http://localhost:${port}`); - console.log(`Swagger documentation: http://localhost:${port}/docs`); + logger.log(`Application is running on: http://localhost:${port}`); + logger.log(`Swagger documentation: http://localhost:${port}/docs`); }As per coding guidelines.
src/modules/reviews/dto/review.dto.ts (1)
3-6: Consolidate duplicate DTO definitions.Two DTOs for creating reviews exist:
CreateReviewDTO(lines 3-6, no decorators) andCreateReviewDto(lines 24-41, with validation decorators). This duplication can lead to confusion and inconsistent validation.Consider removing the undecorated
CreateReviewDTOand using onlyCreateReviewDtowith proper validation decorators throughout the codebase, or clearly document why both are needed.Also applies to: 24-41
src/modules/buyer-requests/tests/buyer-requests.controller.spec.ts (3)
122-142: Inconsistent user ID types in update test.Same issue:
mockRequest.user.idis1(number), service called with1, but expected result hasuserId: "1"(string).Apply consistent typing as suggested in the create test review comment.
147-155: Inconsistent user ID types in remove test.Same issue:
mockRequest.user.idis1(number) and service called with1.Apply consistent typing as suggested in the create test review comment.
160-183: Inconsistent user ID types in close test.Same issue:
mockRequest.user.idis1(number), service called with1, but expected result hasuserId: "1"(string).Apply consistent typing as suggested in the create test review comment.
src/common/filters/http-exception.filter.ts (1)
58-58: Replace directprocess.envaccess with the config object.Accessing
process.env.NODE_ENVdirectly violates the coding guideline to use the config object fromsrc/config/env.As per coding guidelines.
Apply this diff:
+import config from '../../config/env'; // adjust path as needed ... - ...(process.env.NODE_ENV === 'development' && { + ...(config.NODE_ENV === 'development' && {src/middleware/auth.middleware.ts (2)
5-8: Add explicit return type toauthMiddleware.The function lacks an explicit return type, violating the coding guideline.
As per coding guidelines.
Apply this diff:
-export const authMiddleware = (req: Request, res: Response, next: NextFunction) => { +export const authMiddleware = (req: Request, res: Response, next: NextFunction): void => {
10-17: Add explicit return type to middleware function
- Annotate the returned middleware as
: voidto satisfy coding guidelines.- Remove unnecessary
?.onreq.user.rolesincerole: Role[]is always defined.- Optionally simplify
req.user.role.some(role => role === roleName)toreq.user.role.includes(roleName)for clarity.src/config/index.ts (1)
2-28: Coding guideline violation: Directprocess.envaccess.The coding guidelines for
src/**/*.tsrequire importing and using the config object fromsrc/config/envinstead of accessingprocess.envdirectly. This file should import the centralizedENVconstant fromsrc/config/env.ts(mentioned in the AI summary) and reference environment variables through that abstraction.Apply a refactor similar to:
+import { ENV } from './env'; + export const config = { - port: process.env.PORT || 3000, - jwtSecret: process.env.JWT_SECRET || 'your-secret-key', + port: ENV.PORT || 3000, + jwtSecret: ENV.JWT_SECRET || 'your-secret-key', database: { - host: process.env.DB_HOST || 'localhost', - port: parseInt(process.env.DB_PORT || '5432'), - username: process.env.DB_USERNAME || 'postgres', - password: process.env.DB_PASSWORD || 'password', - name: process.env.DB_DATABASE || 'starshop', + host: ENV.DB_HOST || 'localhost', + port: parseInt(ENV.DB_PORT || '5432'), + username: ENV.DB_USERNAME || 'postgres', + password: ENV.DB_PASSWORD || 'password', + name: ENV.DB_DATABASE || 'starshop', synchronize: false, - logging: process.env.NODE_ENV !== 'production', - ssl: process.env.DB_SSL === 'true', + logging: ENV.NODE_ENV !== 'production', + ssl: ENV.DB_SSL === 'true', }, cloudinary: { - cloudName: process.env.CLOUDINARY_CLOUD_NAME, - apiKey: process.env.CLOUDINARY_API_KEY, - apiSecret: process.env.CLOUDINARY_API_SECRET, + cloudName: ENV.CLOUDINARY_CLOUD_NAME, + apiKey: ENV.CLOUDINARY_API_KEY, + apiSecret: ENV.CLOUDINARY_API_SECRET, }, aws: { - accessKeyId: process.env.AWS_ACCESS_KEY_ID, - secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY, - region: process.env.AWS_REGION, - bucketName: process.env.AWS_BUCKET_NAME, + accessKeyId: ENV.AWS_ACCESS_KEY_ID, + secretAccessKey: ENV.AWS_SECRET_ACCESS_KEY, + region: ENV.AWS_REGION, + bucketName: ENV.AWS_BUCKET_NAME, }, supabase: { - url: process.env.SUPABASE_URL, - serviceRoleKey: process.env.SUPABASE_SERVICE_ROLE_KEY, + url: ENV.SUPABASE_URL, + serviceRoleKey: ENV.SUPABASE_SERVICE_ROLE_KEY, },As per coding guidelines.
src/app.module.ts (1)
56-57: Replace direct process.env access with config object.Lines 56, 57, and 86 directly access
process.env, which violates the coding guidelines. Use the config object fromsrc/config/envinstead.The TypeOrmModule.forRoot should be refactored to use ConfigService:
+import { ConfigService } from '@nestjs/config'; + @Module({ imports: [ ConfigModule.forRoot({ isGlobal: true }), ScheduleModule.forRoot(), AppCacheModule, - TypeOrmModule.forRoot({ - type: 'postgres', - url: process.env.DATABASE_URL, - ssl: process.env.DB_SSL === 'true' ? { rejectUnauthorized: false } : undefined, + TypeOrmModule.forRootAsync({ + inject: [ConfigService], + useFactory: (configService: ConfigService) => ({ + type: 'postgres' as const, + url: configService.get<string>('DATABASE_URL'), + ssl: configService.get<string>('DB_SSL') === 'true' ? { rejectUnauthorized: false } : undefined, - entities: [ + entities: [ - /* ... */ + /* ... */ - ], - synchronize: false, - logging: process.env.NODE_ENV === 'development', - }), + ], + synchronize: false, + logging: configService.get<string>('NODE_ENV') === 'development', + }), + }),As per coding guidelines.
Also applies to: 86-86
src/modules/offers/tests/offer.entity.spec.ts (1)
12-23: Fix incorrect assertion: sellerId type mismatch.Line 12 assigns
offer.sellerId = "1"(string), but line 18 assertsexpect(offer.sellerId).toBe(1)(number). This test will fail.Apply this diff:
offer.buyerRequestId = 123; offer.sellerId = "1"; offer.title = 'Test Offer'; offer.description = 'Test offer description'; offer.price = 100.5; expect(offer.buyerRequestId).toBe(123); - expect(offer.sellerId).toBe(1); + expect(offer.sellerId).toBe("1"); expect(offer.title).toBe('Test Offer'); expect(offer.description).toBe('Test offer description'); expect(offer.price).toBe(100.5); expect(offer.status).toBeUndefined(); // default is set by DB });src/common/interceptors/response.interceptor.ts (1)
43-46: Avoidanytype annotation.The
formattedResponsevariable is typed asany, which violates the coding guideline.As per coding guidelines.
Apply this diff:
- const formattedResponse: any = { + const formattedResponse: { success: boolean; data: unknown; token?: string } = { success: true, data, };src/modules/products/services/product.service.ts (1)
45-52: Mark ProductService as injectable before adding injected deps.With the new
CacheServiceconstructor param, Nest will try to resolve this provider. Because the class still lacks@Injectable(), DI metadata isn't emitted and the cache service ends upundefined, breaking every cache call at runtime. ImportInjectableand decorate the class (or wire it differently) so DI can supplyCacheService.-import { CacheService } from '../../../cache/cache.service'; -import { Cacheable, CacheInvalidate } from '../../../cache/decorators/cache.decorator'; +import { Injectable } from '@nestjs/common'; +import { CacheService } from '../../../cache/cache.service'; +import { Cacheable, CacheInvalidate } from '../../../cache/decorators/cache.decorator'; @@ -export class ProductService { +@Injectable() +export class ProductService {src/modules/auth/tests/auth.service.spec.ts (1)
256-263: Update getUserById expectation to use string IDsWith
User.idnow a UUID/string, asserting thatfindOnereceives{ id: 1 }keeps the service bound to the old numeric path. On a UUID column that becomes an invalid query (Postgres will throw “invalid input syntax for type uuid”), so the application will no longer fetch users by ID. Please switch the expectation to the string form (and adjust the service if it still coerces to number):- expect(mockUserRepository.findOne).toHaveBeenCalledWith({ - where: { id: 1 }, + expect(mockUserRepository.findOne).toHaveBeenCalledWith({ + where: { id: '1' }, relations: ['userRoles', 'userRoles.role'], });
🧹 Nitpick comments (32)
src/modules/files/tests/file.controller.spec.ts (1)
87-100: Inconsistent mock data types across test files.This inline
mockUserdefinition has several issues:
ID type inconsistency: Line 88 uses
id: 1(number), butfile.service.spec.tsconsistently usesid: "1"(string). This mismatch can cause confusion and potential test failures.Implicit
anytypes: Lines 95-96 usebuyerData: {}andsellerData: nullwithout explicit types, violating the coding guideline to avoidanytypes.Consider reusing test-utils: The
mockUserfromtest-utils.tsalready defines these fields. Reusing it would reduce duplication and ensure consistency.Apply this diff to align types and consider importing from test-utils:
+import { mockUser as baseMockUser } from './test-utils'; + const mockUser = { - id: 1, + ...baseMockUser, + id: '1', walletAddress: '0x123', name: 'Test User', email: 'test@example.com', password: 'hashed_password', - location: 'Test City', - country: 'Test Country', - buyerData: {}, - sellerData: null, orders: [], userRoles: [], notifications: [], } as unknown as User;src/modules/files/tests/file.service.spec.ts (3)
114-132: Inconsistent field additions across mockUser instances.This
mockUserincludespayoutWallet(line 121), which is not present in other mockUser instances or intest-utils.ts. This inconsistency can lead to confusion about which fields are required for tests.Consider either:
- Adding
payoutWalletto all mockUser instances if it's commonly needed, or- Documenting why this specific test requires it
305-321: Implicitanytypes in mock data.Lines 312-314 use
buyerData: {}andsellerData: nullwithout explicit types. Per coding guidelines: "Do not use the any type; use unknown or specific types instead."Consider adding explicit types:
const mockUser = { id: 1, walletAddress: '0x123', name: 'Test User', email: 'test@example.com', password: 'hashed_password', location: 'Test City', country: 'Test Country', - buyerData: {}, - sellerData: null, + buyerData: {} as Record<string, unknown>, + sellerData: null as unknown, orders: [], userRoles: [], notifications: [], wishlist: [], createdAt: new Date(), updatedAt: new Date(), };
349-365: Implicitanytypes in mock data.Lines 357-358 use
buyerData: {}andsellerData: nullwithout explicit types. Same issue as previous mockUser instances.Apply the same typing fix:
const mockUser = { id: 1, walletAddress: '0x123', name: 'Test User', email: 'test@example.com', password: 'hashed_password', location: 'Test City', country: 'Test Country', - buyerData: {}, - sellerData: null, + buyerData: {} as Record<string, unknown>, + sellerData: null as unknown, orders: [], userRoles: [], notifications: [], wishlist: [], createdAt: new Date(), updatedAt: new Date(), };src/modules/escrows/services/escrow.service.spec.ts (3)
9-25: Refactor to eliminateanyand add explicit return types.The
MockRepoclass usesanyin multiple places (lines 10, 12, 14, 17) and methods lack explicit return types, violating project coding guidelines.Apply this diff to improve type safety:
-class MockRepo<T extends { id?: any }> { +class MockRepo<T extends { id?: string | number }> { private entities: T[] = []; - findOne = jest.fn(async (options: any) => { + findOne = jest.fn(async (options: { where?: { id?: string | number; escrowId?: string }; relations?: string[] }): Promise<T | null> => { if (options.where?.id) return this.entities.find(e => e.id === options.where.id) || null; - if (options.where?.id && options.relations) return this.entities.find(e => e.id === options.where.id) || null; return null; }); - find = jest.fn(async (options: any) => this.entities.filter(e => (options.where?.escrowId ? (e as any).escrowId === options.where.escrowId : true))); + find = jest.fn(async (options: { where?: { escrowId?: string } }): Promise<T[]> => { + return this.entities.filter(e => (options.where?.escrowId ? (e as any).escrowId === options.where.escrowId : true)); + }); - save = jest.fn(async (entity: T) => { + save = jest.fn(async (entity: T): Promise<T> => { const existingIndex = this.entities.findIndex(e => e.id === entity.id); if (existingIndex >= 0) this.entities[existingIndex] = entity; else this.entities.push(entity); return entity; }); - seed(data: T[]) { this.entities = data; } + seed(data: T[]): void { this.entities = data; } }Note: Line 13-14 redundant condition can also be removed (already addressed in diff above).
27-31: Add JSDoc, explicit types, and return type annotation.The
mockTransactionhelper usesanytypes and lacks documentation for its complex transaction-mocking logic, violating coding guidelines.Apply this diff:
+/** + * Mock transaction manager that routes entity operations to the appropriate mock repository. + * @param cb - Callback receiving a mock transaction manager + * @returns The result of the callback execution + */ -const mockTransaction = (cb: any) => cb({ - findOne: (entity: any, opts: any) => entity === Milestone ? milestoneRepo.findOne(opts) : escrowRepo.findOne(opts), - find: (entity: any, opts: any) => milestoneRepo.find(opts), - save: (entity: any) => Array.isArray(entity) ? Promise.all(entity.map(e => (e instanceof Milestone ? milestoneRepo.save(e) : escrowRepo.save(e)))) : (entity instanceof Milestone ? milestoneRepo.save(entity) : escrowRepo.save(entity)) +const mockTransaction = <T>(cb: (manager: any) => Promise<T>): Promise<T> => cb({ + findOne: (entity: any, opts: any): Promise<any> => entity === Milestone ? milestoneRepo.findOne(opts) : escrowRepo.findOne(opts), + find: (entity: any, opts: any): Promise<any[]> => milestoneRepo.find(opts), + save: (entity: any): Promise<any> => Array.isArray(entity) ? Promise.all(entity.map(e => (e instanceof Milestone ? milestoneRepo.save(e) : escrowRepo.save(e)))) : (entity instanceof Milestone ? milestoneRepo.save(entity) : escrowRepo.save(entity)) });
54-58: Consider using proper types instead ofas any.Seed data uses
as anyto bypass type checking. While acceptable in tests, it reduces type safety.If the entities have required fields causing type errors, consider creating proper test fixtures:
// Seed data const mockEscrow: Partial<Escrow> = { id: 'escrow1', sellerId: 10, buyerId: 20 }; const mockMilestone: Partial<Milestone> = { id: 'm1', escrowId: 'escrow1', status: MilestoneStatus.PENDING }; escrowRepo.seed([mockEscrow as Escrow]); milestoneRepo.seed([mockMilestone as Milestone]);Or create factory functions for test entities.
src/modules/seller/dto/build-register.dto.ts (1)
17-32: Add validation decorators to response DTO.The
BuildRegisterResponseDtolacks validation decorators. While response DTOs are primarily for documentation, adding validation ensures consistency and catches serialization issues during development.Apply this diff:
+import { IsString, IsNotEmpty, Matches, IsBoolean, ValidateNested, IsObject } from 'class-validator'; +import { Type } from 'class-transformer'; + +class BuildRegisterDataDto { + @ApiProperty({ + description: 'Unsigned XDR transaction for Soroban contract registration', + example: 'AAAAAgAAAABqjgAAAAAA...', + }) + @IsString() + @IsNotEmpty() + unsignedXdr: string; + + @ApiProperty({ + description: 'Soroban contract address', + example: 'CXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX', + }) + @IsString() + @IsNotEmpty() + contractAddress: string; +} export class BuildRegisterResponseDto { @ApiProperty({ description: 'Success status', example: true, }) + @IsBoolean() success: boolean; - @ApiProperty({ - description: 'Unsigned XDR transaction for Soroban contract registration', - example: 'AAAAAgAAAABqjgAAAAAA...', - }) - data: { - unsignedXdr: string; - contractAddress: string; - }; + @ValidateNested() + @Type(() => BuildRegisterDataDto) + data: BuildRegisterDataDto; }src/main.ts (2)
10-10: Add explicit return type.Per coding guidelines, all functions should have explicit return types. While
Promise<void>is present, ensuring consistency across all functions is important.
13-17: Type the request object for the middleware.The middleware assigns
req._startTimewithout proper typing. This could lead to type safety issues downstream when the interceptor or filter attempts to read this property.Consider adding a type declaration file to extend the Express Request interface:
// src/types/express.d.ts declare namespace Express { export interface Request { _startTime?: number; } }Then ensure this declaration is included in your
tsconfig.json.src/modules/offers/services/offer-attachment.service.ts (2)
28-46: Consider accepting userId as string for consistency.The
userIdparameter is typed asnumberbut compared againstoffer.sellerIdwhich is astring(UUID). While the conversion works, acceptinguserIdasstringfrom the start would be more consistent with the UUID migration.Apply this diff:
async uploadAttachment( offerId: string, file: Express.Multer.File, - userId: number, + userId: string, provider: 'cloudinary' | 's3' = 'cloudinary' ): Promise<OfferAttachmentResponseDto> { // ... - if (offer.sellerId !== userId.toString()) { + if (offer.sellerId !== userId) { throw new ForbiddenException('You can only add attachments to your own offers'); }This aligns with the broader UUID migration and simplifies the comparison logic.
129-141: Consider accepting userId as string for consistency.Same issue as
uploadAttachment: theuserIdparameter is typed asnumberbut compared against a stringsellerId.Apply this diff:
-async deleteAttachment(attachmentId: string, userId: number): Promise<void> { +async deleteAttachment(attachmentId: string, userId: string): Promise<void> { const attachment = await this.offerAttachmentRepository.findOne({ where: { id: attachmentId }, relations: ['offer', 'offer.seller'], }); if (!attachment) { throw new NotFoundException('Attachment not found'); } - if (attachment.offer.sellerId !== userId.toString()) { + if (attachment.offer.sellerId !== userId) { throw new ForbiddenException('You can only delete attachments from your own offers'); }src/common/filters/http-exception.filter.ts (1)
16-17: Type the request with custom properties for_startTime.The request is retrieved without explicit typing, and
_startTimeis a custom property added by middleware but not reflected in the type system.Define an interface extending Express Request:
interface RequestWithTiming extends Request { _startTime?: number; user?: any; // or a proper User type route?: { path?: string }; method?: string; originalUrl?: string; }Then cast or type the request:
- const request = ctx.getRequest(); + const request = ctx.getRequest<RequestWithTiming>();src/modules/escrow/escrow.module.ts (1)
13-20: Consider exporting services if used by other modules.The module does not currently export
EscrowServiceorBlockchainService. If other modules need to inject these services, add them to theexportsarray in the@Moduledecorator.src/modules/buyer-requests/dto/buyer-request-response.dto.ts (1)
3-22: Consider converting interface to class with validators.This response DTO uses an interface, which provides no runtime validation. Per coding guidelines, DTOs should use class-validator decorators. Consider converting
BuyerRequestResponseDtoto a class-based DTO for consistency and runtime type safety.Example structure:
import { Expose, Type } from 'class-transformer'; import { IsString, IsNumber, IsEnum, IsOptional, IsDate, IsBoolean, ValidateNested } from 'class-validator'; class UserResponseDto { @Expose() @IsString() id: string; @Expose() @IsString() name: string; @Expose() @IsString() walletAddress: string; } export class BuyerRequestResponseDto { @Expose() @IsNumber() id: number; @Expose() @IsString() userId: string; @Expose() @IsOptional() @ValidateNested() @Type(() => UserResponseDto) user?: UserResponseDto; // ... remaining fields with appropriate validators }As per coding guidelines.
src/migrations/1751199237000-AddUserFields.ts (1)
6-15: Consider adding indexes for location and country columns.If these columns will be used in WHERE clauses or for filtering users by location/country, consider adding indexes to improve query performance.
Apply this diff to add indexes:
public async up(queryRunner: QueryRunner): Promise<void> { // Add new columns to users table await queryRunner.query(` ALTER TABLE "users" ADD COLUMN "location" character varying NULL, ADD COLUMN "country" character varying NULL, ADD COLUMN "buyerData" jsonb NULL, ADD COLUMN "sellerData" jsonb NULL `); + + // Add indexes for common query patterns + await queryRunner.query(` + CREATE INDEX "IDX_users_country" ON "users" ("country") WHERE "country" IS NOT NULL + `); + await queryRunner.query(` + CREATE INDEX "IDX_users_location" ON "users" ("location") WHERE "location" IS NOT NULL + `); }And update the
down()method:public async down(queryRunner: QueryRunner): Promise<void> { + // Drop indexes first + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_users_location"`); + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_users_country"`); + // Remove the columns await queryRunner.query(` ALTER TABLE "users" DROP COLUMN "location", DROP COLUMN "country", DROP COLUMN "buyerData", DROP COLUMN "sellerData" `); }src/modules/disputes/controllers/dispute.controller.ts (1)
12-19: Wrap service call in try-catch for proper error handling.The controller does not handle errors from the service layer, which could result in unhandled exceptions.
async startDispute( @Body() dto: StartDisputeDto, @Req() req: AuthenticatedRequest ): Promise<any> { + try { const buyer = req.user; return this.disputeService.startDispute(dto.orderItemId, buyer, dto.reason); + } catch (error) { + // Let NestJS exception filters handle it, or add custom logging + throw error; + } }Alternatively, rely on NestJS global exception filters if they are configured. If custom error handling or logging is needed, add it here.
As per coding guidelines.
src/modules/offers/tests/offer.entity.spec.ts (1)
25-35: Inconsistent test assertion on line 33.This test validates negative price behavior but does not relate to the sellerId type change. However, ensure the assertion is meaningful: testing that
offer.priceis less than 0 after assigning -50 is trivial.Consider renaming or clarifying the test purpose, or remove if redundant:
- it('should create an offer with price validation', () => { + it('should allow negative price in memory (would fail in DB)', () => { const offer = new Offer(); offer.buyerRequestId = 123; offer.sellerId = "1"; offer.title = 'Test Offer'; offer.description = 'Test offer description'; offer.price = -50; // Invalid in DB, but allowed in-memory expect(offer.price).toBeLessThan(0); });src/modules/escrow/services/escrow.service.spec.ts (1)
39-63: Add descriptive test names and consider edge cases.The tests cover basic success and error cases, which is good. Consider adding tests for:
- Invalid amount (e.g., negative, zero)
- Concurrent funding attempts
- Escrow already funded
Example additional test:
it('throws BadRequestException for zero or negative amount', async () => { const repo = dataSource.getRepository(Escrow); const escrow = repo.create({ expectedSigner: 'SIGNER1', balance: '0' }); await repo.save(escrow); await expect( service.fundEscrow(escrow.id, { signer: 'SIGNER1', amount: 0 }) ).rejects.toBeInstanceOf(BadRequestException); });src/config/env.ts (2)
1-29: Consider Zod schema validation per coding guidelines.The coding guideline specifies: "Validate all environment variables in the centralized env module using a Zod schema." This implementation uses
env-var, which provides runtime validation but differs from the prescribed approach.As per coding guidelines.
While
env-vardoes offer validation (.required(),.asPortNumber(), etc.), adopting Zod would align with the team's standard and provide additional benefits like static type inference.Example Zod-based approach:
import { z } from 'zod'; const envSchema = z.object({ NODE_ENV: z.string().default('development'), PORT: z.coerce.number().int().positive().default(3000), DATABASE_URL: z.string().min(1), JWT_SECRET: z.string().min(1), SUPABASE_URL: z.string().url(), SUPABASE_ANON_KEY: z.string().min(1), REDIS_URL: z.string().url().default('redis://localhost:6379'), HORIZON_URL: z.string().url().default('https://horizon-testnet.stellar.org'), STELLAR_NETWORK: z.string().default('testnet'), EMAIL_SERVICE: z.string().default('gmail'), EMAIL_USER: z.string().email(), EMAIL_PASSWORD: z.string().min(1), BASE_URL: z.string().url().default('http://localhost:3000'), }); export const ENV = envSchema.parse(process.env); export type Env = z.infer<typeof envSchema>;
3-29: Add explicit type annotation for ENV.The
ENVconstant lacks an explicit type, reducing type safety and IDE autocomplete support.Apply this diff to add a type:
+interface EnvConfig { + NODE_ENV: string; + PORT: number; + DATABASE_URL: string; + JWT_SECRET: string; + SUPABASE_URL: string; + SUPABASE_ANON_KEY: string; + REDIS_URL: string; + HORIZON_URL: string; + STELLAR_NETWORK: string; + EMAIL_SERVICE: string; + EMAIL_USER: string; + EMAIL_PASSWORD: string; + BASE_URL: string; +} + -export const ENV = { +export const ENV: EnvConfig = { NODE_ENV: env.get("NODE_ENV").default("development").asString(),src/modules/auth/dto/auth.dto.ts (1)
14-16: Remove unused import.
Typeis imported fromclass-transformerbut never used—please drop it to keep the file clean.src/modules/seller/dto/submit-register.dto.ts (1)
24-29: Consider extracting the nested data object to a separate class.The inline nested object shape is acceptable, but extracting it to a separate class would improve reusability and type safety if this shape is used elsewhere.
Example refactor:
export class RegisterResultDto { @ApiProperty({ description: 'Transaction hash' }) transactionHash: string; @ApiProperty({ description: 'Contract ID' }) contractId: string; @ApiProperty({ description: 'Payout wallet address' }) payoutWallet: string; @ApiProperty({ description: 'Registration status' }) registered: boolean; } export class SubmitRegisterResponseDto { @ApiProperty({ description: 'Success status', example: true, }) success: boolean; @ApiProperty({ description: 'Registration result with transaction hash', type: RegisterResultDto, }) data: RegisterResultDto; }src/migrations/1756199900000-CreateEscrowTables.ts (1)
7-12: Consider adding an index on expected_signer.If escrows will be frequently queried by signer, add an index to improve performance.
CREATE INDEX IF NOT EXISTS escrows_expected_signer_idx ON escrows(expected_signer);src/modules/auth/controllers/auth.controller.ts (1)
148-154: Consider validating country code format in RegisterUserDto.While the
toUpperCase()transformation is good, ensure the DTO validates that country is a valid ISO 3166-1 alpha-2 code using@IsISO31661Alpha2()decorator from class-validator.In
src/modules/auth/dto/auth.dto.ts(RegisterUserDto):import { IsOptional, IsISO31661Alpha2 } from 'class-validator'; @ApiProperty({ description: 'User country code', example: 'US', required: false, }) @IsOptional() @IsISO31661Alpha2() country?: string;src/cache/controllers/cache.controller.ts (1)
20-22: Unnecessary await.Return the Promise directly; drop async/await here.
- async getStats() { - return await this.cacheService.getStats(); - } + getStats(): Promise<unknown> { + return this.cacheService.getStats(); + }As per coding guidelines.
src/modules/orders/entities/1695840100000-AddMilestoneAndStatusToOrderItem.ts (1)
6-10: Migration looks correct; consider idempotency safeguards.Optional: add IF NOT EXISTS where supported to reduce re-run fragility in mixed environments.
If targeting Postgres ≥ 9.6, you can use:
ALTER TABLE "order_items" ADD COLUMN IF NOT EXISTS "milestone" varchar(255); DO $$ BEGIN CREATE TYPE "order_item_status_enum" AS ENUM('ACTIVE','DISPUTED','COMPLETED'); EXCEPTION WHEN duplicate_object THEN NULL; END $$; ALTER TABLE "order_items" ADD COLUMN IF NOT EXISTS "status" "order_item_status_enum" NOT NULL DEFAULT 'ACTIVE';src/cache/decorators/cache.decorator.ts (2)
35-40: Export and reuse metadata keys; type the decorator.Avoid magic strings to prevent drift across interceptor/guards.
+export const CACHE_INVALIDATE_METADATA = 'cache_invalidate'; -export const CacheInvalidate = (entity: string, action?: string) => { - return (target: any, propertyKey: string, descriptor: PropertyDescriptor) => { - SetMetadata('cache_invalidate', { entity, action })(target, propertyKey, descriptor); +export const CacheInvalidate = (entity: string, action?: string): MethodDecorator => { + return (target: object, propertyKey: string | symbol, descriptor: PropertyDescriptor) => { + SetMetadata(CACHE_INVALIDATE_METADATA, { entity, action })(target, propertyKey, descriptor); return descriptor; }; };
45-50: Same for CacheClear: avoid any and magic strings.+export const CACHE_CLEAR_METADATA = 'cache_clear'; -export const CacheClear = () => { - return (target: any, propertyKey: string, descriptor: PropertyDescriptor) => { - SetMetadata('cache_clear', true)(target, propertyKey, descriptor); +export const CacheClear = (): MethodDecorator => { + return (target: object, propertyKey: string | symbol, descriptor: PropertyDescriptor) => { + SetMetadata(CACHE_CLEAR_METADATA, true)(target, propertyKey, descriptor); return descriptor; }; };As per coding guidelines.
src/cache/interceptors/cache.interceptor.ts (2)
29-31: Support class-level metadata with getAllAndOverride.This allows applying Cacheable at controller level.
- const cacheKeyMetadata = this.reflector.get(CACHE_KEY_METADATA, handler); - const cacheTtlMetadata = this.reflector.get(CACHE_TTL_METADATA, handler); + const cacheKeyMetadata = this.reflector.getAllAndOverride( + CACHE_KEY_METADATA, + [handler, context.getClass()], + ); + const cacheTtlMetadata = this.reflector.getAllAndOverride( + CACHE_TTL_METADATA, + [handler, context.getClass()], + );
24-52: Optional: guard cache I/O with try/catch to degrade gracefully.Prevent cache outages from breaking requests; log at warn.
src/modules/escrow/tests/escrow.service.spec.ts (1)
24-99: Type the mocked transaction manager instead ofanyLine 24 currently declares
transactionManager: any, which breaks our no-anyrule and prevents the compiler from catching mistakes in the mocked transaction helpers. Type it as anEntityManagermock so misuse is caught during compilation.Apply this diff to bring it in line:
-import { Repository, DataSource } from 'typeorm'; +import { Repository, DataSource, EntityManager } from 'typeorm'; ... - let transactionManager: any; + let transactionManager: jest.Mocked<Partial<EntityManager>>; ... - transactionManager = { - findOne: jest.fn(), - save: jest.fn(), - create: jest.fn(), - }; + transactionManager = { + findOne: jest.fn(), + save: jest.fn(), + create: jest.fn(), + } as jest.Mocked<Partial<EntityManager>>;As per coding guidelines.
| "cache-manager": "^7.2.2", | ||
| "cache-manager-redis-yet": "^5.1.5", |
There was a problem hiding this comment.
Critical: Incompatible cache-manager versions.
cache-manager ^7.2.2 is incompatible with cache-manager-redis-yet ^5.1.5. The latter is a legacy store adapter for cache-manager v5, while v7.x has migrated to Keyv-backed stores.
This will cause runtime failures when the Redis store attempts to register with cache-manager v7.
Based on learnings
Apply this diff to use compatible versions:
- "cache-manager": "^7.2.2",
- "cache-manager-redis-yet": "^5.1.5",
+ "cache-manager": "^5.7.6",
+ "cache-manager-redis-yet": "^5.1.5",OR migrate to the modern Keyv-backed approach:
"cache-manager": "^7.2.2",
- "cache-manager-redis-yet": "^5.1.5",
+ "@keyv/redis": "^3.0.1",
+ "keyv": "^5.1.1",Then update src/cache/cache.module.ts to use the Keyv Redis adapter instead of the legacy store.
📝 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.
| "cache-manager": "^7.2.2", | |
| "cache-manager-redis-yet": "^5.1.5", | |
| // package.json, around lines 54–55 | |
| "cache-manager": "^5.7.6", | |
| "cache-manager-redis-yet": "^5.1.5", |
| "cache-manager": "^7.2.2", | |
| "cache-manager-redis-yet": "^5.1.5", | |
| // package.json, around lines 54–55 | |
| "cache-manager": "^7.2.2", | |
| "@keyv/redis": "^3.0.1", | |
| "keyv": "^5.1.1", |
🤖 Prompt for AI Agents
In package.json around lines 54-55, the project declares cache-manager ^7.2.2
and cache-manager-redis-yet ^5.1.5 which are incompatible; either replace the
redis adapter with a Keyv-backed store or align versions: Option A — change
package.json to use cache-manager ^5.x and keep cache-manager-redis-yet ^5.1.5
then run install and verify src/cache/cache.module.ts still registers the legacy
store; Option B (preferred) — remove cache-manager-redis-yet and add Keyv and
keyv-redis (and any Keyv typings), update package.json accordingly, then modify
src/cache/cache.module.ts to initialize Keyv with the Redis adapter (import
Keyv, import KeyvRedis, instantiate Keyv with the Redis adapter and connection
options) and replace cache-manager registration with the new Keyv-based cache
client, update any code that used cache-manager API to the Keyv API, run tests
and lint to ensure no runtime type errors.
| import { Escrow } from './modules/escrows/entities/escrow.entity'; | ||
| import { Milestone } from './modules/escrows/entities/milestone.entity'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify escrow vs escrows module naming.
There appear to be two separate escrow-related modules: EscrowModule (from ./modules/escrow/escrow.module) and EscrowsModule (from ./modules/escrows/escrows.module). This naming is confusing and suggests potential architectural duplication.
Run the following script to understand the distinction:
Consider consolidating into a single module if they serve similar purposes, or rename one to better reflect its distinct responsibility (e.g., EscrowAccountsModule vs EscrowTransactionsModule).
🏁 Script executed:
#!/bin/bash
# Description: Investigate the difference between escrow and escrows modules
echo "=== Checking escrow module structure ==="
echo "--- ./modules/escrow ---"
fd . src/modules/escrow -t f -x echo {/}
echo -e "\n--- ./modules/escrows ---"
fd . src/modules/escrows -t f -x echo {/}
echo -e "\n=== Checking exports from each module ==="
echo "--- escrow.module.ts ---"
rg -A 10 "export.*class.*EscrowModule" src/modules/escrow/escrow.module.ts
echo -e "\n--- escrows.module.ts ---"
rg -A 10 "export.*class.*EscrowsModule" src/modules/escrows/escrows.module.ts
echo -e "\n=== Checking for potential conflicts ==="
rg -l "EscrowModule|EscrowsModule" src --type tsLength of output: 1104
Consolidate duplicate escrow modules.
The escrow (src/modules/escrow) and escrows (src/modules/escrows) modules both define overlapping entities (escrow.entity.ts, milestone.entity.ts), services, controllers, and DTOs—this duplication is confusing. Merge them into a single module or rename one to clearly reflect a distinct domain responsibility (e.g., EscrowAccountsModule vs EscrowMilestonesModule).
🤖 Prompt for AI Agents
In src/app.module.ts around lines 45-46, the code imports Escrow and Milestone
from the duplicated escrows/escrow modules; consolidate the duplicate modules by
choosing one of two approaches: either merge the contents of src/modules/escrow
and src/modules/escrows into a single module (move entities, services,
controllers, DTOs into one folder, update all import paths in app.module.ts and
across the codebase, register only the unified module and entities in Nest
module metadata, and delete the now-empty duplicate folder) or rename one module
to reflect a distinct domain (e.g., EscrowAccounts vs EscrowMilestones) and
refactor its exports and import paths accordingly so there is no overlapping
entity/service/controller; after making the change, update app.module.ts imports
to reference the final module paths, adjust any tests or DI bindings that
referenced the old paths, and run the test suite and a full TypeScript build to
ensure no unresolved imports remain.
| EscrowAccount, | ||
| Milestone, | ||
|
|
||
| Escrow, | ||
| EscrowFundingTx, | ||
| Store, | ||
| Escrow, | ||
| Milestone, | ||
| ], |
There was a problem hiding this comment.
Remove duplicate entity registrations.
Escrow and Milestone are registered twice in the entities array (lines 77 & 82, and 79 & 83), which is redundant and may cause unexpected behavior.
Apply this diff to remove duplicates:
EscrowAccount,
Milestone,
-
- Escrow,
EscrowFundingTx,
Store,
- Escrow,
- Milestone,
],📝 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.
| EscrowAccount, | |
| Milestone, | |
| Escrow, | |
| EscrowFundingTx, | |
| Store, | |
| Escrow, | |
| Milestone, | |
| ], | |
| EscrowAccount, | |
| Milestone, | |
| EscrowFundingTx, | |
| Store, | |
| ], |
🤖 Prompt for AI Agents
In src/app.module.ts around lines 76 to 84, the entities array includes
duplicate registrations of Escrow and Milestone (appearing twice); remove the
duplicate entries so each entity appears only once in the array, leaving a
single Escrow and a single Milestone declaration and preserving the other
entities (EscrowAccount, EscrowFundingTx, Store) in place.
| EscrowModule, | ||
| SupabaseModule, | ||
| EscrowModule, | ||
| StoresModule, | ||
| EscrowsModule, |
There was a problem hiding this comment.
Remove duplicate EscrowModule import.
EscrowModule is imported twice (lines 101 and 103), which will cause a module initialization error.
Apply this diff:
OffersModule,
EscrowModule,
SupabaseModule,
- EscrowModule,
StoresModule,
EscrowsModule,📝 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.
| EscrowModule, | |
| SupabaseModule, | |
| EscrowModule, | |
| StoresModule, | |
| EscrowsModule, | |
| OffersModule, | |
| EscrowModule, | |
| SupabaseModule, | |
| StoresModule, | |
| EscrowsModule, |
🤖 Prompt for AI Agents
In src/app.module.ts around lines 101 to 105 there is a duplicate import of
EscrowModule (appears on lines 101 and 103); remove the second occurrence (line
103) from the module's imports array so EscrowModule is only imported once,
leaving the other imports (SupabaseModule, StoresModule, EscrowsModule) intact.
| import * as redisStore from 'cache-manager-redis-yet'; | ||
| import { CacheService } from './cache.service'; | ||
| import { CacheController } from './controllers/cache.controller'; | ||
| import { JwtService } from '@nestjs/jwt'; | ||
| import { Role, RoleService, UserRole } from '@/modules/auth'; | ||
| import { TypeOrmModule } from '@nestjs/typeorm'; | ||
|
|
||
| @Module({ | ||
| imports: [ | ||
| TypeOrmModule.forFeature([Role, UserRole]), | ||
| CacheModule.registerAsync({ | ||
| isGlobal: true, | ||
| imports: [ConfigModule], | ||
| useFactory: async (configService: ConfigService) => { | ||
| const redisUrl = configService.get<string>('REDIS_URL'); | ||
| const ttl = parseInt(configService.get<string>('CACHE_TTL_SECONDS') ?? '60', 10); | ||
| const prefix = configService.get<string>('CACHE_PREFIX') ?? 'app:'; | ||
|
|
||
| if (!redisUrl) { | ||
| throw new Error('REDIS_URL environment variable is required for caching'); | ||
| } | ||
|
|
||
| return { | ||
| store: redisStore, | ||
| url: redisUrl, | ||
| ttl, | ||
| prefix, | ||
| retryStrategy: (times: number) => { | ||
| const delay = Math.min(times * 50, 2000); | ||
| return delay; | ||
| }, | ||
| maxRetriesPerRequest: 3, | ||
| }; |
There was a problem hiding this comment.
Fix Redis store registration (await the factory).
cache-manager-redis-yet exports an async redisStore factory; passing the function itself means Nest receives { store: Function }, so cache operations explode at runtime (get/set are undefined). Import the named export and await it before returning the cache options.
-import * as redisStore from 'cache-manager-redis-yet';
+import { redisStore } from 'cache-manager-redis-yet';
@@
- return {
- store: redisStore,
- url: redisUrl,
- ttl,
- prefix,
- retryStrategy: (times: number) => {
- const delay = Math.min(times * 50, 2000);
- return delay;
- },
- maxRetriesPerRequest: 3,
- };
+ return {
+ store: await redisStore({
+ url: redisUrl,
+ ttl,
+ keyPrefix: prefix,
+ retryStrategy: (times: number) => Math.min(times * 50, 2000),
+ maxRetriesPerRequest: 3,
+ }),
+ ttl,
+ };🤖 Prompt for AI Agents
In src/cache/cache.module.ts around lines 4 to 36, the Redis store is being
passed as the async factory function itself (so Nest receives { store: Function
} and cache methods are undefined); import the named async factory export from
'cache-manager-redis-yet' (not the whole module), call and await that factory
inside the useFactory, assign the awaited result to the store property in the
returned options, and keep the other options
(url/ttl/prefix/retryStrategy/maxRetriesPerRequest) unchanged.
| approve( | ||
| @Param('escrowId') escrowId: string, | ||
| @Param('milestoneId') milestoneId: string, | ||
| @Request() req: AuthRequest | ||
| ) { | ||
| return this.escrowService.approveMilestone(escrowId, milestoneId, Number(req.user.id)); | ||
| } | ||
|
|
||
| @Patch(':escrowId/milestones/:milestoneId/status') | ||
| @UseGuards(JwtAuthGuard, RolesGuard) | ||
| @Roles(Role.SELLER) | ||
| changeStatus( | ||
| @Param('escrowId') escrowId: string, | ||
| @Param('milestoneId') milestoneId: string, | ||
| @Body() body: UpdateMilestoneStatusDto, | ||
| @Request() req: AuthRequest | ||
| ) { | ||
| return this.escrowService.changeMilestoneStatus( | ||
| escrowId, | ||
| milestoneId, | ||
| Number(req.user.id), | ||
| body.status as MilestoneStatus | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Annotate controller method return types.
Per our src/**/*.ts guidelines we need explicit return types on every method. Please update approve and changeStatus to return the actual Promise<...> type (e.g., the DTO returned by EscrowService) so the controller complies.
🤖 Prompt for AI Agents
In src/modules/escrows/controllers/escrow.controller.ts lines 18-41, the
controller methods approve and changeStatus lack explicit return types; update
their signatures to return the concrete Promise<T> types that EscrowService
methods return (import the appropriate DTO/response types from the service or
DTO files and annotate approve as Promise<...> and changeStatus as
Promise<...>), ensure required imports are added at the top, and run typecheck
to fix any mismatches—do not use plain Promise<any>, use the exact DTO/response
types returned by EscrowService.
| import { IsEnum } from 'class-validator'; | ||
| import { MilestoneStatus } from '../entities/milestone.entity'; | ||
|
|
||
| // Seller-changeable statuses (not including APPROVED which is buyer action) | ||
| export class UpdateMilestoneStatusDto { | ||
| @IsEnum(MilestoneStatus, { message: 'Invalid milestone status' }) | ||
| status: MilestoneStatus; // Expect READY | IN_PROGRESS | DELIVERED | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add OpenAPI decorator for API documentation.
The DTO correctly validates the status field using @IsEnum, but it's missing OpenAPI documentation decorators. Since the project uses @nestjs/swagger (per dependencies), DTOs should include @ApiProperty decorators for consistent API documentation generation.
Apply this diff to add OpenAPI documentation:
import { IsEnum } from 'class-validator';
+import { ApiProperty } from '@nestjs/swagger';
import { MilestoneStatus } from '../entities/milestone.entity';
// Seller-changeable statuses (not including APPROVED which is buyer action)
export class UpdateMilestoneStatusDto {
@IsEnum(MilestoneStatus, { message: 'Invalid milestone status' })
+ @ApiProperty({
+ enum: MilestoneStatus,
+ description: 'New milestone status (seller-changeable only: READY, IN_PROGRESS, DELIVERED)',
+ })
status: MilestoneStatus; // Expect READY | IN_PROGRESS | DELIVERED
}As per coding guidelines.
📝 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.
| import { IsEnum } from 'class-validator'; | |
| import { MilestoneStatus } from '../entities/milestone.entity'; | |
| // Seller-changeable statuses (not including APPROVED which is buyer action) | |
| export class UpdateMilestoneStatusDto { | |
| @IsEnum(MilestoneStatus, { message: 'Invalid milestone status' }) | |
| status: MilestoneStatus; // Expect READY | IN_PROGRESS | DELIVERED | |
| } | |
| import { IsEnum } from 'class-validator'; | |
| import { ApiProperty } from '@nestjs/swagger'; | |
| import { MilestoneStatus } from '../entities/milestone.entity'; | |
| // Seller-changeable statuses (not including APPROVED which is buyer action) | |
| export class UpdateMilestoneStatusDto { | |
| @IsEnum(MilestoneStatus, { message: 'Invalid milestone status' }) | |
| @ApiProperty({ | |
| enum: MilestoneStatus, | |
| description: 'New milestone status (seller-changeable only: READY, IN_PROGRESS, DELIVERED)', | |
| }) | |
| status: MilestoneStatus; // Expect READY | IN_PROGRESS | DELIVERED | |
| } |
🤖 Prompt for AI Agents
In src/modules/escrows/dto/update-milestone-status.dto.ts lines 1-8, the DTO is
missing OpenAPI metadata; add an import for ApiProperty from '@nestjs/swagger'
and annotate the status property with @ApiProperty({ enum: MilestoneStatus,
description: 'Milestone status', example: MilestoneStatus.READY }) so the
generated Swagger docs include the enum, description and an example; keep the
existing @IsEnum validation intact.
| const createdFile: File = { | ||
| id: 'uuid', | ||
| url: mockFile.path, | ||
| type: FileType.IMAGE, | ||
| filename: mockFile.originalname, | ||
| mimetype: mockFile.mimetype, | ||
| size: mockFile.size, | ||
| type: FileType.IMAGE, | ||
| url: mockFile.path, | ||
| providerType: 'cloudinary', | ||
| providerPublicId: mockFile.filename, | ||
| uploadedById: '1', | ||
| uploadedAt: new Date(), | ||
| uploadedById: '1', | ||
| uploadedBy: mockUser, | ||
| // uploadedById: mockUser.id.toString(), | ||
| // uploadedAt: new Date(), | ||
| // uploadedBy: mockUser, | ||
| } as File; |
There was a problem hiding this comment.
Remove commented-out code.
Lines 89-91 contain commented-out code. Per coding guidelines: "Do not leave commented-out code in commits."
Apply this diff to remove the commented code:
const createdFile: File = {
id: 'uuid',
filename: mockFile.originalname,
mimetype: mockFile.mimetype,
size: mockFile.size,
type: FileType.IMAGE,
url: mockFile.path,
providerType: 'cloudinary',
providerPublicId: mockFile.filename,
uploadedAt: new Date(),
uploadedById: '1',
uploadedBy: mockUser,
- // uploadedById: mockUser.id.toString(),
- // uploadedAt: new Date(),
- // uploadedBy: mockUser,
} as File;📝 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.
| const createdFile: File = { | |
| id: 'uuid', | |
| url: mockFile.path, | |
| type: FileType.IMAGE, | |
| filename: mockFile.originalname, | |
| mimetype: mockFile.mimetype, | |
| size: mockFile.size, | |
| type: FileType.IMAGE, | |
| url: mockFile.path, | |
| providerType: 'cloudinary', | |
| providerPublicId: mockFile.filename, | |
| uploadedById: '1', | |
| uploadedAt: new Date(), | |
| uploadedById: '1', | |
| uploadedBy: mockUser, | |
| // uploadedById: mockUser.id.toString(), | |
| // uploadedAt: new Date(), | |
| // uploadedBy: mockUser, | |
| } as File; | |
| const createdFile: File = { | |
| id: 'uuid', | |
| filename: mockFile.originalname, | |
| mimetype: mockFile.mimetype, | |
| size: mockFile.size, | |
| type: FileType.IMAGE, | |
| url: mockFile.path, | |
| providerType: 'cloudinary', | |
| providerPublicId: mockFile.filename, | |
| uploadedAt: new Date(), | |
| uploadedById: '1', | |
| uploadedBy: mockUser, | |
| } as File; |
🤖 Prompt for AI Agents
In src/modules/files/tests/file.service.spec.ts around lines 77 to 92, there are
three commented-out lines (previously at lines 89-91) left in the createdFile
object literal; remove those commented lines so the test contains only active
properties (delete the lines "// uploadedById: mockUser.id.toString(),", "//
uploadedAt: new Date(),", and "// uploadedBy: mockUser,") and save the file with
no leftover commented-out code.
| location: 'Test City', | ||
| country: 'Test Country', | ||
| buyerData: {}, | ||
| sellerData: null, |
There was a problem hiding this comment.
Type inconsistency and missing explicit types for mock data.
Several issues with the new fields:
-
ID type mismatch: Line 3 uses
id: 1(number), butfile.service.spec.tsconsistently usesid: "1"(string) for mockUser. This inconsistency can lead to test failures or false positives. -
Implicit
anytype:buyerData: {}andsellerData: nulllack explicit types. Per coding guidelines, avoid implicitanytypes.
Apply this diff to align with the string ID convention and add explicit types:
export const mockUser = {
- id: 1,
+ id: '1',
walletAddress: '0x123456789abcdef',
name: 'Test User',
email: 'test@example.com',
location: 'Test City',
country: 'Test Country',
- buyerData: {},
- sellerData: null,
+ buyerData: {} as Record<string, unknown>,
+ sellerData: null as unknown,
};📝 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.
| location: 'Test City', | |
| country: 'Test Country', | |
| buyerData: {}, | |
| sellerData: null, | |
| export const mockUser = { | |
| id: '1', | |
| walletAddress: '0x123456789abcdef', | |
| name: 'Test User', | |
| email: 'test@example.com', | |
| location: 'Test City', | |
| country: 'Test Country', | |
| buyerData: {} as Record<string, unknown>, | |
| sellerData: null as unknown, | |
| }; |
🤖 Prompt for AI Agents
In src/modules/files/tests/test-utils.ts around lines 7 to 10, change the id
field from a number to a string to match the "1" convention used across tests,
and replace the implicit-any mock fields by giving buyerData an explicit typed
shape (for example Record<string, unknown> or the actual BuyerData interface)
and sellerData an explicit nullable type (e.g., Record<string, unknown> | null
or SellerData | null) so both fields are typed rather than implicitly any.
| @Expose() | ||
| escrow_contract_id?: string; | ||
|
|
||
| @Expose() | ||
| payment_tx_hash?: string; | ||
|
|
||
| @Expose() | ||
| onchain_status?: OnchainStatus; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation decorators to the new optional fields.
The new blockchain-related fields lack validation decorators. Per coding guidelines, DTOs should use class-validator decorators for data validation.
Apply this diff to add proper validation:
+import { IsOptional, IsString, IsEnum } from 'class-validator';
+
export class OrderDto {
// ... existing fields ...
@Expose()
+ @IsOptional()
+ @IsString()
escrow_contract_id?: string;
@Expose()
+ @IsOptional()
+ @IsString()
payment_tx_hash?: string;
@Expose()
+ @IsOptional()
+ @IsEnum(OnchainStatus)
onchain_status?: OnchainStatus;As per coding guidelines.
📝 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.
| @Expose() | |
| escrow_contract_id?: string; | |
| @Expose() | |
| payment_tx_hash?: string; | |
| @Expose() | |
| onchain_status?: OnchainStatus; | |
| import { Expose } from 'class-transformer'; | |
| import { IsOptional, IsString, IsEnum } from 'class-validator'; | |
| export class OrderDto { | |
| // ... existing fields ... | |
| @Expose() | |
| @IsOptional() | |
| @IsString() | |
| escrow_contract_id?: string; | |
| @Expose() | |
| @IsOptional() | |
| @IsString() | |
| payment_tx_hash?: string; | |
| @Expose() | |
| @IsOptional() | |
| @IsEnum(OnchainStatus) | |
| onchain_status?: OnchainStatus; | |
| } |
🤖 Prompt for AI Agents
In src/modules/orders/dto/order.dto.ts around lines 31 to 38, the three new
optional blockchain fields lack class-validator decorators; add validation by
decorating escrow_contract_id and payment_tx_hash with @IsOptional() and
@IsString(), and decorate onchain_status with @IsOptional() and
@IsEnum(OnchainStatus). Also ensure you import IsOptional, IsString, and IsEnum
from 'class-validator' (and keep the existing @Expose imports) so the DTO
validates these fields correctly at runtime.
1 participant
Issue #202
🔧 Fix Tests & Dependencies
Changes
✅ Outcome
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests