feat(bookings): implement POST /api/bookings endpoint for reservation creation #73
Conversation
…tion, Soroban availability check, Trustless Work escrow, and Supabase persistence
WalkthroughThis update introduces a complete backend booking creation workflow. It adds blockchain-based property availability checks via Soroban, escrow management through TrustlessWork, and database integration for booking records. New API endpoints, controller logic, validation schemas, and comprehensive tests are implemented, with authentication and error handling throughout the booking process. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMiddleware
participant BookingController
participant BookingService
participant SorobanModule
participant TrustlessWork
participant Database
Client->>AuthMiddleware: POST /api/bookings (with JWT)
AuthMiddleware->>BookingController: Pass authenticated request
BookingController->>BookingService: createBooking(input)
BookingService->>SorobanModule: checkAvailability(propertyId, dates)
SorobanModule-->>BookingService: AvailabilityResponse
alt Available
BookingService->>TrustlessWork: createEscrow(booking/payment details)
TrustlessWork-->>BookingService: Escrow address
BookingService->>Database: Insert booking record
Database-->>BookingService: Booking ID
BookingService-->>BookingController: Booking data
BookingController-->>Client: 201 Created + booking
else Not available
BookingService-->>BookingController: Error (UNAVAILABLE)
BookingController-->>Client: 409 Conflict
end
Possibly related issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (1)
apps/backend/src/tests/booking.test.ts (1)
94-101: Update mock response to match userId formatSame userId format issue in the mock response data.
🧹 Nitpick comments (3)
apps/backend/src/types/booking.types.ts (1)
20-24: Consider adjusting date validation for same-day bookingsThe current validation might reject valid same-day bookings. Consider using start of day for comparison to allow bookings for any time today.
from: z .date() - .refine((date) => date >= new Date(), { + .refine((date) => { + const today = new Date(); + today.setHours(0, 0, 0, 0); + return date >= today; + }, { - message: 'The start date must be in the future', + message: 'The start date cannot be in the past', }),apps/backend/src/blockchain/soroban.ts (1)
65-65: Consider making the transaction fee configurable.The hardcoded fee of '100' stroops might be insufficient during network congestion. Consider making this configurable via environment variables.
+ const baseFee = process.env.STELLAR_BASE_FEE || '1000'; const transaction = new TransactionBuilder(account, { - fee: '100', + fee: baseFee,apps/backend/src/blockchain/trustlessWork.ts (1)
353-358: Consider extracting USDC issuer addresses as named constants.For better maintainability and clarity, consider defining these addresses as named constants.
+const USDC_ISSUERS = { + mainnet: 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN', + testnet: 'GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5', +} as const; + function getUSDCIssuer(): string { if (STELLAR_NETWORK === 'mainnet') { - return 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN'; + return USDC_ISSUERS.mainnet; } - return 'GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5'; + return USDC_ISSUERS.testnet; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/backend/package.json(1 hunks)apps/backend/src/blockchain/soroban.ts(1 hunks)apps/backend/src/blockchain/trustlessWork.ts(1 hunks)apps/backend/src/controllers/bookingControllers.ts(1 hunks)apps/backend/src/index.ts(2 hunks)apps/backend/src/routes/booking.routes.ts(1 hunks)apps/backend/src/services/booking.service.ts(1 hunks)apps/backend/src/tests/booking.test.ts(1 hunks)apps/backend/src/types/booking.types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/backend/src/routes/booking.routes.ts (2)
apps/backend/src/middleware/auth.middleware.ts (1)
authenticateToken(5-31)apps/backend/src/controllers/bookingControllers.ts (1)
postBooking(6-46)
apps/backend/src/controllers/bookingControllers.ts (2)
apps/backend/src/types/booking.types.ts (1)
createBookingSchema(15-36)apps/backend/src/services/booking.service.ts (1)
createBooking(9-67)
apps/backend/src/blockchain/trustlessWork.ts (1)
apps/backend/src/tests/booking.test.ts (1)
createEscrow(36-47)
🪛 Biome (1.9.4)
apps/backend/src/tests/booking.test.ts
[error] 36-36: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🔇 Additional comments (1)
apps/backend/src/blockchain/trustlessWork.ts (1)
1-415: Well-structured escrow integration with TrustlessWork API.The module provides a comprehensive integration with proper error handling, validation, and a clean API surface. The separation between low-level client operations and high-level manager functions follows good design principles.
| "test": "bun test" | ||
| }, | ||
| "dependencies": { | ||
| "@stellar/stellar-sdk": "^13.3.0", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the package versions for security and compatibility.
Ensure that the specified versions of the Stellar SDK and Soroban client are the latest stable versions and free from known vulnerabilities.
Also applies to: 23-23
🏁 Script executed:
#!/bin/bash
# Description: Check for latest versions and security advisories for the new blockchain dependencies
echo "Checking @stellar/stellar-sdk versions..."
curl -s https://registry.npmjs.org/@stellar/stellar-sdk | jq '.["dist-tags"].latest'
echo "Checking soroban-client versions..."
curl -s https://registry.npmjs.org/soroban-client | jq '.["dist-tags"].latest'
# Check for security advisories
echo "Checking for security advisories..."
npm audit --audit-level moderate --json | jq '.advisories'Length of output: 852
Dependencies Up-to-Date; Add Lockfile & Run Security Audit
The specified versions of @stellar/stellar-sdk (13.3.0) and soroban-client (1.0.1) already match the latest stable releases on npm. However, without a lockfile npm audit cannot report vulnerabilities:
• File: apps/backend/package.json
– Line 15: "@stellar/stellar-sdk": "^13.3.0"
– Line 23: "soroban-client": "^1.0.1"
Recommendations:
- Generate a lockfile (e.g.
npm install --package-lock-only). - Run
npm audit(or integrate CI dependency scanning). - Review and remediate any vulnerabilities before merging.
🤖 Prompt for AI Agents
In apps/backend/package.json at lines 15 and 23, the versions for
@stellar/stellar-sdk and soroban-client are up-to-date, but there is no lockfile
present to ensure consistent installs and enable vulnerability auditing. To fix
this, generate a package-lock.json by running `npm install --package-lock-only`
in the apps/backend directory, then run `npm audit` to check for vulnerabilities
and address any issues before merging.
| import { rateLimiter } from './middleware/rateLimiter'; | ||
| import authRoutes from './routes/auth'; | ||
|
|
||
| import bookingRoutes from './routes/booking.routes'; |
There was a problem hiding this comment.
Potential route path duplication issue.
The booking routes are registered at /api/bookings in the main app, but the route definition in booking.routes.ts (line 7) also includes /api/bookings. This will result in a doubled path /api/bookings/api/bookings.
Apply this fix to the route registration:
-app.use('/api/bookings', bookingRoutes);
+app.use('/api', bookingRoutes);Or alternatively, update the route definition in booking.routes.ts to use just / instead of /api/bookings.
Also applies to: 33-33
🤖 Prompt for AI Agents
In apps/backend/src/index.ts at line 7 and also line 33, the booking routes are
imported and registered with a base path of /api/bookings, but the
booking.routes.ts file also prefixes its routes with /api/bookings, causing the
full route to be duplicated as /api/bookings/api/bookings. To fix this, either
remove the /api/bookings prefix from the route definitions inside
booking.routes.ts and use just / as the base path there, or change the route
registration in index.ts to register bookingRoutes at the root path / instead of
/api/bookings. This will prevent the route path duplication.
|
|
||
| const router = Router(); | ||
|
|
||
| router.post('/api/bookings', authenticateToken, postBooking); |
There was a problem hiding this comment.
Fix the route path to avoid duplication.
The route is defined as /api/bookings, but this router is mounted at /api/bookings in index.ts (line 33), resulting in the final path /api/bookings/api/bookings.
Apply this fix:
-router.post('/api/bookings', authenticateToken, postBooking);
+router.post('/', authenticateToken, postBooking);This ensures the final endpoint will be correctly accessible at POST /api/bookings.
📝 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.
| router.post('/api/bookings', authenticateToken, postBooking); | |
| router.post('/', authenticateToken, postBooking); |
🤖 Prompt for AI Agents
In apps/backend/src/routes/booking.routes.ts at line 7, the route path is
defined as '/api/bookings' which duplicates the base path set in index.ts line
33. To fix this, remove the '/api/bookings' prefix from the route and change it
to just '/' so that when mounted, the final path is correctly '/api/bookings'
for the POST request.
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | ||
| if ((error as any).code === 'UNAVAILABLE') { | ||
| return res.status(409).json({ error: error.message }); | ||
| } | ||
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | ||
| if ((error as any).code === 'ESCROW_FAIL') { | ||
| return res.status(500).json({ error: error.message }); | ||
| } | ||
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | ||
| if ((error as any).code === 'DB_FAIL') { | ||
| return res.status(500).json({ error: error.message }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for error handling.
The use of any type assertions compromises type safety. Define proper error types or use type guards.
Create a custom error type:
interface ServiceError extends Error {
code?: string;
status?: number;
}
// Then use type guards instead of any assertions:
const isServiceError = (error: unknown): error is ServiceError => {
return error instanceof Error && ('code' in error || 'status' in error);
};
// In the catch block:
if (isServiceError(error)) {
if (error.code === 'UNAVAILABLE') {
return res.status(409).json({ error: error.message });
}
if (error.status) {
return res.status(error.status).json({ error: error.message });
}
}🤖 Prompt for AI Agents
In apps/backend/src/controllers/bookingControllers.ts around lines 32 to 43,
replace the use of 'any' type assertions for error handling with a proper custom
error type and type guard. Define an interface for the error that includes
optional 'code' and 'status' properties, then implement a type guard function to
check if the error matches this interface. Use this type guard in the catch
block to safely access error properties and handle different error codes and
statuses without compromising type safety.
Fix error handling inconsistencies with service layer.
The error handling logic doesn't match the actual error structure from the service. Based on the service implementation in apps/backend/src/services/booking.service.ts, errors are thrown with different structures.
Looking at the service code (lines 34 and 49), errors are thrown as objects with status and message properties, not with code properties. Apply this fix:
- // biome-ignore lint/suspicious/noExplicitAny: <explanation>
- if ((error as any).code === 'UNAVAILABLE') {
- return res.status(409).json({ error: error.message });
- }
- // biome-ignore lint/suspicious/noExplicitAny: <explanation>
- if ((error as any).code === 'ESCROW_FAIL') {
- return res.status(500).json({ error: error.message });
- }
- // biome-ignore lint/suspicious/noExplicitAny: <explanation>
- if ((error as any).code === 'DB_FAIL') {
- return res.status(500).json({ error: error.message });
- }
+ // Handle custom service errors
+ if (error && typeof error === 'object' && 'code' in error) {
+ if (error.code === 'UNAVAILABLE') {
+ return res.status(409).json({ error: error.message });
+ }
+ }
+
+ // Handle service errors with status property
+ if (error && typeof error === 'object' && 'status' in error) {
+ return res.status(error.status).json({ error: error.message });
+ }📝 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.
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |
| if ((error as any).code === 'UNAVAILABLE') { | |
| return res.status(409).json({ error: error.message }); | |
| } | |
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |
| if ((error as any).code === 'ESCROW_FAIL') { | |
| return res.status(500).json({ error: error.message }); | |
| } | |
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |
| if ((error as any).code === 'DB_FAIL') { | |
| return res.status(500).json({ error: error.message }); | |
| } | |
| // Handle custom service errors | |
| if (error && typeof error === 'object' && 'code' in error) { | |
| if (error.code === 'UNAVAILABLE') { | |
| return res.status(409).json({ error: error.message }); | |
| } | |
| } | |
| // Handle service errors with status property | |
| if (error && typeof error === 'object' && 'status' in error) { | |
| return res.status(error.status).json({ error: error.message }); | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/controllers/bookingControllers.ts around lines 32 to 43, the
error handling checks for an error.code property, but the service layer throws
errors with status and message properties instead. Update the error handling to
check error.status instead of error.code, and use error.message for the response
message. Adjust the status codes returned to match error.status values from the
service, ensuring consistency with the service's error structure.
| const input = createBookingSchema.parse({ | ||
| ...req.body, | ||
| dates: { | ||
| from: new Date(req.body.dates.from), | ||
| to: new Date(req.body.dates.to), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve date parsing with validation.
The manual conversion of date strings to Date objects can fail silently with invalid dates, potentially causing validation errors downstream.
Apply this improvement:
const input = createBookingSchema.parse({
...req.body,
dates: {
- from: new Date(req.body.dates.from),
- to: new Date(req.body.dates.to),
+ from: req.body.dates?.from ? new Date(req.body.dates.from) : undefined,
+ to: req.body.dates?.to ? new Date(req.body.dates.to) : undefined,
},
});This prevents runtime errors when req.body.dates is undefined and lets Zod handle the validation.
📝 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 input = createBookingSchema.parse({ | |
| ...req.body, | |
| dates: { | |
| from: new Date(req.body.dates.from), | |
| to: new Date(req.body.dates.to), | |
| }, | |
| }); | |
| const input = createBookingSchema.parse({ | |
| ...req.body, | |
| dates: { | |
| from: req.body.dates?.from ? new Date(req.body.dates.from) : undefined, | |
| to: req.body.dates?.to ? new Date(req.body.dates.to) : undefined, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In apps/backend/src/controllers/bookingControllers.ts around lines 12 to 18, the
current code manually converts date strings to Date objects, which can silently
fail if the dates are invalid or if req.body.dates is undefined. To fix this,
remove the manual Date conversion and pass req.body directly to
createBookingSchema.parse, allowing Zod to handle date validation and catch
invalid or missing dates properly.
| const conflictingBookings = | ||
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | ||
| availabilityData.conflicting_bookings?.map((booking: any) => ({ | ||
| bookingId: booking.id, | ||
| dates: { | ||
| from: new Date(booking.from_timestamp * 1000), | ||
| to: new Date(booking.to_timestamp * 1000), | ||
| }, | ||
| })) || []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for contract response handling.
The use of any type reduces type safety. Consider defining a proper interface for the contract response.
+ interface ContractBooking {
+ id: string;
+ from_timestamp: number;
+ to_timestamp: number;
+ }
+
const conflictingBookings =
- // biome-ignore lint/suspicious/noExplicitAny: <explanation>
- availabilityData.conflicting_bookings?.map((booking: any) => ({
+ availabilityData.conflicting_bookings?.map((booking: ContractBooking) => ({
bookingId: booking.id,
dates: {
from: new Date(booking.from_timestamp * 1000),
to: new Date(booking.to_timestamp * 1000),
},
})) || [];📝 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 conflictingBookings = | |
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |
| availabilityData.conflicting_bookings?.map((booking: any) => ({ | |
| bookingId: booking.id, | |
| dates: { | |
| from: new Date(booking.from_timestamp * 1000), | |
| to: new Date(booking.to_timestamp * 1000), | |
| }, | |
| })) || []; | |
| interface ContractBooking { | |
| id: string; | |
| from_timestamp: number; | |
| to_timestamp: number; | |
| } | |
| const conflictingBookings = | |
| availabilityData.conflicting_bookings?.map((booking: ContractBooking) => ({ | |
| bookingId: booking.id, | |
| dates: { | |
| from: new Date(booking.from_timestamp * 1000), | |
| to: new Date(booking.to_timestamp * 1000), | |
| }, | |
| })) || []; |
🤖 Prompt for AI Agents
In apps/backend/src/blockchain/soroban.ts around lines 92 to 100, the use of
'any' for the booking parameter reduces type safety. Define a proper TypeScript
interface representing the structure of a booking object returned by the
contract, including fields like id, from_timestamp, and to_timestamp. Then
replace the 'any' type annotation with this interface in the map function to
improve type safety and clarity.
| export async function createEscrowWithDetails( | ||
| params: BookingEscrowParams | ||
| ): Promise<EscrowResult> { | ||
| try { | ||
| const escrowAddress = await createEscrow(params); | ||
|
|
||
| return { | ||
| escrowId: '', | ||
| escrowAddress, | ||
| contractId: '', | ||
| status: 'created', | ||
| totalAmount: params.totalAmount.toFixed(2), | ||
| success: true, | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
Return complete escrow details from the API response.
The function returns empty escrowId and contractId fields, which defeats the purpose of a "WithDetails" variant. The underlying createEscrow function only returns the address, not the full response.
Consider refactoring to return the complete response:
export async function createEscrowWithDetails(
params: BookingEscrowParams
): Promise<EscrowResult> {
try {
- const escrowAddress = await createEscrow(params);
+ const client = new TrustlessWorkClient();
+ const escrowRequest = buildEscrowRequest(params); // Extract request building logic
+ const response = await client.createEscrow(escrowRequest);
return {
- escrowId: '',
- escrowAddress,
- contractId: '',
- status: 'created',
+ escrowId: response.escrowId,
+ escrowAddress: response.escrowAddress,
+ contractId: response.contractId,
+ status: response.status,
totalAmount: params.totalAmount.toFixed(2),
success: true,
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/backend/src/blockchain/trustlessWork.ts around lines 326 to 340, the
createEscrowWithDetails function currently returns empty escrowId and contractId
fields because createEscrow only returns the escrow address. To fix this,
refactor the function to call an updated or alternative method that returns the
full escrow details including escrowId and contractId, then extract and return
these complete details in the response object instead of empty strings.
| try { | ||
| Keypair.fromPublicKey(request.buyer); | ||
| } catch { | ||
| errors.push('Invalid buyer Stellar address'); | ||
| } |
There was a problem hiding this comment.
Align error message format with test expectations.
Based on the test mocks, the error for invalid buyer address should match the expected format.
The test expects the exact error message "Validation errors: Invalid buyer Stellar address" when validation fails. Update to match:
try {
Keypair.fromPublicKey(request.buyer);
} catch {
- errors.push('Invalid buyer Stellar address');
+ if (!request.buyer) {
+ errors.push('Buyer address is required');
+ } else {
+ errors.push('Invalid buyer Stellar address');
+ }
}📝 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.
| try { | |
| Keypair.fromPublicKey(request.buyer); | |
| } catch { | |
| errors.push('Invalid buyer Stellar address'); | |
| } | |
| try { | |
| Keypair.fromPublicKey(request.buyer); | |
| } catch { | |
| if (!request.buyer) { | |
| errors.push('Buyer address is required'); | |
| } else { | |
| errors.push('Invalid buyer Stellar address'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/blockchain/trustlessWork.ts around lines 200 to 204, the
error message pushed to errors for an invalid buyer Stellar address does not
match the expected test format. Update the error message string to exactly
"Validation errors: Invalid buyer Stellar address" to align with the test
expectations.
| description: `Escrow for property rental booking from ${ | ||
| params.dates.from.toISOString().split('T')[0] | ||
| } to ${params.dates.to.toISOString().split('T')[0]} for ${ | ||
| params.guests | ||
| } guest(s). Total: ${params.totalAmount.toFixed( | ||
| 2 | ||
| )} (includes ${params.depositAmount.toFixed(2)} security deposit).`, | ||
| metadata: { | ||
| propertyId: params.propertyId, | ||
| bookingId: params.bookingId, | ||
| dates: { | ||
| from: params.dates.from.toISOString().split('T')[0] ?? '', | ||
| to: params.dates.to.toISOString().split('T')[0] ?? '', | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve date formatting reliability.
The current implementation uses optional chaining with empty string fallback, which could hide date formatting issues.
+ const fromDateStr = params.dates.from.toISOString().split('T')[0];
+ const toDateStr = params.dates.to.toISOString().split('T')[0];
+
description: `Escrow for property rental booking from ${
- params.dates.from.toISOString().split('T')[0]
- } to ${params.dates.to.toISOString().split('T')[0]} for ${
+ fromDateStr
+ } to ${toDateStr} for ${
params.guests
} guest(s). Total: ${params.totalAmount.toFixed(
2
)} (includes ${params.depositAmount.toFixed(2)} security deposit).`,
metadata: {
propertyId: params.propertyId,
bookingId: params.bookingId,
dates: {
- from: params.dates.from.toISOString().split('T')[0] ?? '',
- to: params.dates.to.toISOString().split('T')[0] ?? '',
+ from: fromDateStr,
+ to: toDateStr,
},
guests: params.guests,
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/backend/src/blockchain/trustlessWork.ts around lines 282 to 295, the
date formatting uses optional chaining with a fallback to an empty string, which
may mask errors if the date is invalid or missing. Instead, explicitly check if
the date objects exist and are valid before formatting them to ISO strings, and
handle invalid or missing dates with proper error handling or default values to
ensure reliability and avoid silent failures.
| if (!request.amount || Number.parseFloat(request.amount) <= 0) { | ||
| errors.push('Valid amount is required'); | ||
| } |
There was a problem hiding this comment.
Add error handling for amount parsing.
The parseFloat operation could throw or return NaN for invalid input. Add proper validation.
- if (!request.amount || Number.parseFloat(request.amount) <= 0) {
- errors.push('Valid amount is required');
+ if (!request.amount) {
+ errors.push('Amount is required');
+ } else {
+ const parsedAmount = Number.parseFloat(request.amount);
+ if (isNaN(parsedAmount) || parsedAmount <= 0) {
+ errors.push('Amount must be greater than 0');
+ }
}📝 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.
| if (!request.amount || Number.parseFloat(request.amount) <= 0) { | |
| errors.push('Valid amount is required'); | |
| } | |
| if (!request.amount) { | |
| errors.push('Amount is required'); | |
| } else { | |
| const parsedAmount = Number.parseFloat(request.amount); | |
| if (isNaN(parsedAmount) || parsedAmount <= 0) { | |
| errors.push('Amount must be greater than 0'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/blockchain/trustlessWork.ts around lines 192 to 194, the
current code uses Number.parseFloat on request.amount without handling cases
where it returns NaN or throws an error. Update the validation to safely parse
the amount, check if the result is a valid number greater than zero, and add an
error to the errors array if the amount is invalid or cannot be parsed.
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
Thank you for the work done in this PR. Before proceeding with the merge, I would recommend making a few important adjustments. First, it would be ideal to refactor the code to use proper dependency injection for the blockchain-related services, as this will facilitate testing and improve maintainability. It is also necessary to implement the standard response wrapper used in the application to ensure consistency in the API responses. Additionally, I would ask you to add more comprehensive error handling, including rollback mechanisms where applicable. Improving test coverage—especially for failure scenarios—is important to ensure the system's robustness. Finally, you should incorporate proper logging and monitoring for blockchain operations. I also noticed that CodeRabbit suggested some corrections; please review and apply them to ensure everything is functioning correctly and we can proceed with merging your code.
Lastly, make sure to pull the latest changes from the target branch using git pull --rebase and resolve any conflicts, so the branch is up to date before merging.
@ebubechi-ihediwa any updates? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/backend/src/services/booking.service.ts (1)
36-47: Critical: Missing required escrow parameters not addressed.The escrow creation still has empty values for required fields (
bookingId,sellerAddress,propertyTitle), which was flagged in previous reviews. This will cause escrow creation to fail or create invalid contracts.You need to:
- Generate or retrieve a valid
bookingIdbefore callingcreateEscrow- Query the properties table to get
sellerAddressandpropertyTitleusinginput.propertyId- Ensure all required fields are populated before creating the escrow
Refer to the previous review suggestion for the complete implementation.
🧹 Nitpick comments (2)
apps/backend/src/services/logging.service.ts (1)
24-31: Potential type safety issue with details casting.The type assertion
(log.details as Record<string, unknown>)on line 28 could fail ifdetailsis not an object. Consider safer type handling.Use a safer approach:
public logBlockchainSuccess(log: TransactionLog, result: unknown) { const successLog: TransactionLog = { ...log, status: 'completed', - details: { ...(log.details as Record<string, unknown>), result }, + details: typeof log.details === 'object' && log.details !== null + ? { ...(log.details as Record<string, unknown>), result } + : { originalDetails: log.details, result }, }; this.logTransaction(successLog); }apps/backend/src/types/test.types.ts (1)
40-44: Remove redundant field definitions in TestBookingData.The
TestBookingDatainterface redefinesid,status, andescrowAddressfields that are already present in theBookinginterface it extends. This creates unnecessary duplication.Simplify the interface:
-export interface TestBookingData extends Booking { - id: string; - status: 'pending' | 'confirmed' | 'cancelled' | 'completed'; - escrowAddress: string; -} +export interface TestBookingData extends Booking { + // All required fields are inherited from Booking interface +}Or if you need to override specific field types, only redefine those that differ:
export interface TestBookingData extends Booking { - id: string; - status: 'pending' | 'confirmed' | 'cancelled' | 'completed'; - escrowAddress: string; + // Only override if the type differs from Booking interface }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)apps/backend/src/services/booking.service.ts(1 hunks)apps/backend/src/services/logging.service.ts(1 hunks)apps/backend/src/tests/booking.test.ts(1 hunks)apps/backend/src/types/common.types.ts(1 hunks)apps/backend/src/types/test.types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- apps/backend/src/types/common.types.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/backend/src/services/logging.service.ts (1)
apps/backend/src/types/common.types.ts (1)
TransactionLog(31-37)
apps/backend/src/types/test.types.ts (4)
apps/backend/src/blockchain/soroban.ts (2)
AvailabilityRequest(117-117)AvailabilityResponse(117-117)apps/backend/src/blockchain/trustlessWork.ts (1)
BookingEscrowParams(226-240)apps/backend/src/types/booking.types.ts (1)
Booking(2-14)apps/backend/src/types/common.types.ts (1)
TransactionLog(31-37)
🔇 Additional comments (12)
apps/backend/src/tests/booking.test.ts (4)
17-27: Well-structured test data with proper UUID format.The mock booking data now uses valid UUIDs for both
propertyIdanduserId, addressing the previous schema validation concerns. The date generation ensures future dates for proper validation.
94-102: Comprehensive success case validation.The test properly validates the core booking creation flow and verifies all expected return values including bookingId, escrowAddress, and status.
106-129: Excellent coverage of availability conflict scenarios.The test properly simulates property unavailability with conflicting bookings and validates that the correct BookingError is thrown with appropriate error code and conflict details.
131-138: Proper escrow failure handling test.The test correctly simulates escrow creation failure and validates error propagation with the appropriate error code.
apps/backend/src/services/booking.service.ts (2)
8-15: Excellent dependency injection implementation.The
BlockchainServicesinterface and constructor injection pattern significantly improve testability and maintainability, addressing the feedback about dependency injection from the PR comments.
65-96: Robust error handling and rollback mechanism.The implementation properly handles database failures with escrow rollback, maintains error context through logging, and uses consistent error types. The rollback logic ensures data consistency by attempting escrow cancellation when database operations fail.
apps/backend/src/services/logging.service.ts (3)
4-6: Clean and readable console logging implementation.The pretty-printed JSON format will be helpful for development and debugging.
8-11: Good foundation with clear production path.The TODO comment clearly indicates the next step for production deployment. The current implementation serves development needs well.
43-43: Appropriate singleton pattern for logging service.The singleton export pattern works well for a stateless logging service.
apps/backend/src/types/test.types.ts (3)
9-13: Well-typed blockchain service mocks.The mock interfaces properly mirror the actual
BlockchainServicesinterface with correct Mock typing from Bun test framework.
15-30: Accurate Supabase client mock structure.The chained method structure correctly mirrors how the Supabase client is used in the booking service, ensuring realistic test scenarios.
33-37: Consistent logging service mock types.The mock types properly match the
LoggingServiceinterface with appropriate Mock typing and return types.
| it('should attempt to cancel escrow when database operation fails', async () => { | ||
| const dbError = new Error('Database error'); | ||
| Object.assign(supabase, { | ||
| from: () => ({ | ||
| insert: () => ({ | ||
| select: () => ({ | ||
| single: () => Promise.resolve({ data: null, error: dbError }), | ||
| }), | ||
| }), | ||
| }), | ||
| }); | ||
|
|
||
| await expect(bookingService.createBooking(validBookingData)).rejects.toThrow( | ||
| new BookingError('Failed to create booking record', 'DB_FAIL', dbError) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing verification of escrow cancellation during rollback.
The test simulates database failure but doesn't verify that cancelEscrow was actually called as part of the rollback mechanism. Add verification to ensure rollback behavior is working correctly.
Add verification after the test assertion:
await expect(bookingService.createBooking(validBookingData)).rejects.toThrow(
new BookingError('Failed to create booking record', 'DB_FAIL', dbError)
);
+
+ // Verify that escrow cancellation was attempted during rollback
+ expect(mockBlockchainServices.cancelEscrow).toHaveBeenCalledWith('test-escrow-address');📝 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.
| it('should attempt to cancel escrow when database operation fails', async () => { | |
| const dbError = new Error('Database error'); | |
| Object.assign(supabase, { | |
| from: () => ({ | |
| insert: () => ({ | |
| select: () => ({ | |
| single: () => Promise.resolve({ data: null, error: dbError }), | |
| }), | |
| }), | |
| }), | |
| }); | |
| await expect(bookingService.createBooking(validBookingData)).rejects.toThrow( | |
| new BookingError('Failed to create booking record', 'DB_FAIL', dbError) | |
| ); | |
| }); | |
| it('should attempt to cancel escrow when database operation fails', async () => { | |
| const dbError = new Error('Database error'); | |
| Object.assign(supabase, { | |
| from: () => ({ | |
| insert: () => ({ | |
| select: () => ({ | |
| single: () => Promise.resolve({ data: null, error: dbError }), | |
| }), | |
| }), | |
| }), | |
| }); | |
| await expect(bookingService.createBooking(validBookingData)).rejects.toThrow( | |
| new BookingError('Failed to create booking record', 'DB_FAIL', dbError) | |
| ); | |
| // Verify that escrow cancellation was attempted during rollback | |
| expect(mockBlockchainServices.cancelEscrow).toHaveBeenCalledWith('test-escrow-address'); | |
| }); |
🤖 Prompt for AI Agents
In apps/backend/src/tests/booking.test.ts around lines 159 to 174, the test
simulates a database failure but does not verify that the cancelEscrow function
is called during rollback. To fix this, add a mock or spy on cancelEscrow before
the test runs, then after the expect assertion, verify that cancelEscrow was
called to ensure the rollback behavior is properly tested.
| it('should handle failed escrow cancellation during rollback', async () => { | ||
| const dbError = new Error('Database error'); | ||
| const rollbackError = new Error('Failed to cancel escrow'); | ||
|
|
||
| mockBlockchainServices.cancelEscrow = mock(() => Promise.reject(rollbackError)); | ||
|
|
||
| Object.assign(supabase, { | ||
| from: () => ({ | ||
| insert: () => ({ | ||
| select: () => ({ | ||
| single: () => Promise.resolve({ data: null, error: dbError }), | ||
| }), | ||
| }), | ||
| }), | ||
| }); | ||
|
|
||
| await expect(bookingService.createBooking(validBookingData)).rejects.toThrow( | ||
| new BookingError('Failed to create booking record', 'DB_FAIL', dbError) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing verification of failed rollback scenario.
Similar to the previous test, this should verify that escrow cancellation was attempted even when it fails, and that the original database error is still thrown (not the rollback error).
Add verification after the test assertion:
await expect(bookingService.createBooking(validBookingData)).rejects.toThrow(
new BookingError('Failed to create booking record', 'DB_FAIL', dbError)
);
+
+ // Verify that escrow cancellation was attempted during rollback
+ expect(mockBlockchainServices.cancelEscrow).toHaveBeenCalledWith('test-escrow-address');📝 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.
| it('should handle failed escrow cancellation during rollback', async () => { | |
| const dbError = new Error('Database error'); | |
| const rollbackError = new Error('Failed to cancel escrow'); | |
| mockBlockchainServices.cancelEscrow = mock(() => Promise.reject(rollbackError)); | |
| Object.assign(supabase, { | |
| from: () => ({ | |
| insert: () => ({ | |
| select: () => ({ | |
| single: () => Promise.resolve({ data: null, error: dbError }), | |
| }), | |
| }), | |
| }), | |
| }); | |
| await expect(bookingService.createBooking(validBookingData)).rejects.toThrow( | |
| new BookingError('Failed to create booking record', 'DB_FAIL', dbError) | |
| ); | |
| }); | |
| it('should handle failed escrow cancellation during rollback', async () => { | |
| const dbError = new Error('Database error'); | |
| const rollbackError = new Error('Failed to cancel escrow'); | |
| mockBlockchainServices.cancelEscrow = mock(() => Promise.reject(rollbackError)); | |
| Object.assign(supabase, { | |
| from: () => ({ | |
| insert: () => ({ | |
| select: () => ({ | |
| single: () => Promise.resolve({ data: null, error: dbError }), | |
| }), | |
| }), | |
| }), | |
| }); | |
| await expect(bookingService.createBooking(validBookingData)).rejects.toThrow( | |
| new BookingError('Failed to create booking record', 'DB_FAIL', dbError) | |
| ); | |
| // Verify that escrow cancellation was attempted during rollback | |
| expect(mockBlockchainServices.cancelEscrow).toHaveBeenCalledWith('test-escrow-address'); | |
| }); |
🤖 Prompt for AI Agents
In apps/backend/src/tests/booking.test.ts around lines 176 to 195, the test does
not verify that the escrow cancellation was attempted despite the rollback
failure, nor does it confirm that the original database error is thrown instead
of the rollback error. After the expect assertion, add a verification step to
check that mockBlockchainServices.cancelEscrow was called, and ensure the thrown
error matches the original database error, not the rollback error.
2 participants
This PR introduces the new POST /api/bookings endpoint, allowing authenticated users to create property reservations in StellarRent. Key features include:
Testing Instructions:
{
"propertyId": "uuid-of-property",
"userId": "uuid-of-user",
"dates": { "from": "2025-07-01", "to": "2025-07-05" },
"guests": 2,
"total": 400.00,
"deposit": 100.00
}
Verify responses for:
Run unit tests: npm run test
Checklist:
Summary by CodeRabbit