review.md

Code Review: Synthetix Orderbook API

Executive Summary

This code review identifies critical issues in the Synthetix Orderbook API codebase that significantly impact maintainability, security, and reliability. The most severe issues include lack of type safety in Redis operations, extensive code duplication, security vulnerabilities, and architectural inconsistencies.

Critical Issues

1. Redis Usage Without Type Safety

Severity: High

The codebase uses Redis extensively without any type safety mechanisms:

• No type validation: All Redis operations return any type
• String-based field access: Error-prone and refactor-unfriendly
• No schema validation: Data integrity cannot be guaranteed
• Manual serialization: JSON.stringify/parse without type checking

Specific Examples:

// src/endpoints/fapi/v1/order/orderController.ts:147-152
const order = fold(await rdb.bookOrderStatus(
  RKEY_BOOK_ORDERS(body.symbol, req.keyAccountId),
  orderId,
));
const executedQty = order['executedQty'] || 0; // No type checking, accessing with string
const quantity = order['quantity']; // Could be undefined

// src/endpoints/api/order.ts:354
// @ts-expect-error need to update interface
batch.bookProcessActivity(RKEY_BOOK_ACTIVITY(body.symbol), Date.now());

// src/endpoints/fapi/v1/order/orderController.ts:420
(batch as any).bookProcessActivity( // Cast to any to bypass type checking
  RKEY_BOOK_ACTIVITY(body.symbol),
  Date.now(),
);

More Locations:

• src/endpoints/fapi/v1/order/orderController.ts:649 - Another (batch as any) cast
• src/endpoints/fapi/v1/order/orderController.ts:794 - Same pattern repeated
• src/utils/requestValidation.ts - fold() function returns any

Impact:

• Runtime errors from accessing non-existent fields
• Data corruption from incorrect field types
• Difficult to refactor or maintain

Recommendation:

• Implement typed Redis client wrapper with generics
• Add schema validation layer (e.g., Zod)
• Create type-safe key builders and serializers

2. Extensive Code Duplication

Severity: High

Significant code duplication across the codebase:

Specific Examples:

1. Order Response Formatting (Duplicated 3+ times):

// src/endpoints/fapi/v1/order/orderController.ts:163-191
{
  type: formatOrderType(order),
  side: formatOrderSide(order),
  origQty: formatQuantity(order['quantity']),
  executedQty: formatQuantity(order['executedQty']),
  cumQuote: formatQuantity(order['cumulativeQuoteQty']),
  price: formatQuantity(order['price']),
  timeInForce: formatOrderTimeInForce(order),
  // ... 20+ more fields
}

// EXACT SAME structure in:
// - src/endpoints/fapi/v1/order/orderController.ts:577-605
// - src/endpoints/fapi/v1/order/orderController.ts:674-701
// - src/endpoints/fapi/v1/order/index.ts:129-167

2. Time Range Validation (Duplicated):

// src/endpoints/fapi/v1/order/orderController.ts:514-533
if (startTime) {
  const now = new Date();
  const weekAgo = new Date();
  weekAgo.setDate(now.getDate() - 7);
  
  if (new Date(startTime) < weekAgo) {
    throw new ExpectError(['startTime'], 'the query time period must be less then 7 days');
  }
}

// SAME validation in:
// - src/endpoints/fapi/v1/order/index.ts:758-775

3. Redis Stream Processing Pattern:

// src/endpoints/fapi/v1/order/orderController.ts:638-713
stream.on('data', (currentOrders: string[]) => {
  stream.pause();
  const pipeline = rdb.pipeline();
  // ... processing logic
  pipeline.exec()
    .then(() => stream.resume())
    .catch((err) => reject(new Error(JSON.stringify(err))));
});

// SAME pattern in:
// - src/endpoints/fapi/v1/order/orderController.ts:542-619
// - src/endpoints/api/order.ts:multiple locations

4. Hardcoded TODO Values (Repeated everywhere):

// Found in multiple order response objects:
workingType: 'CONTRACT_PRICE', // TODO,
positionSide: 'SHORT', // TODO,
closePosition: false, // TODO
time: Date.now(), // TODO
updateTime: Date.now(), // TODO
activatePrice: '0', // TODO
avgPrice: '0', // TODO

Impact:

• Increased maintenance burden
• Inconsistent behavior when updates are made
• Higher risk of bugs

Recommendation:

• Extract common functionality into shared utilities
• Create base classes for controllers
• Implement factory functions for response objects

3. Security Vulnerabilities

Severity: Critical

Specific Security Issues:

1. Weak JWT Secret Configuration:

// src/config.ts:6
export const JWT_SECRET = process.env.JWT_SECRET || 'random_secret';
// Predictable secret in development allows token forgery

2. Signature Verification Bypass Risk:

// src/endpoints/fapi/v1/order/orderController.ts:250-264
const isValid = await verifySignature(order, inputs.address, req.body.signature, chainId, contracts.PerpsMarket.address);
const signedOrder: SignedBookOrderWithSignature = {
  ...order,
  signature: req.body.signature, // Order created before validation completes
};
if (!isValid) {
  throw new ExpectError(['signature'], 'is invalid or malformed');
}

3. Permissive CORS Configuration:

// src/app.ts:44-49
app.use(cors({
  origin: true, // Allows ALL origins
  credentials: true,
  exposedHeaders: ['X-MBX-USED-WEIGHT', 'X-MBX-ORDER-COUNT-10S'],
}));

4. Console Logging in Production:

// src/endpoints/fapi/v1/order/orderController.ts:629
console.log('hello'); // Debug log left in production

// src/endpoints/fapi/v3/token/tokenController.ts:95
console.error('something went wrong when validating the signature:', err);

// src/endpoints/api/order.ts:560
console.error('error on discard', err);

5. No Rate Limiting Implementation:

• No rate limiting middleware found in src/app.ts
• Vulnerable to brute force on authentication endpoints
• No protection against DDoS attacks

6. Missing Security Headers:

• No helmet.js or equivalent security headers middleware
• Missing X-Content-Type-Options, X-Frame-Options, etc.

7. Error Stack Traces Exposed:

// src/app.ts:66-90
app.use((err: unknown, req: express.Request, res: express.Response, next: express.NextFunction) => {
  // Stack traces potentially exposed in non-production
  if (process.env.NODE_ENV !== 'production') {
    res.status(status).json({
      message,
      fields,
      stack: err.stack, // Exposes internal details
    });
  }
});

Impact:

• Authentication bypass possible
• API abuse and resource exhaustion
• Cross-origin attacks
• Information disclosure

Recommendation:

• Use environment-specific JWT secrets
• Add rate limiting (express-rate-limit)
• Restrict CORS to specific origins
• Add helmet.js for security headers
• Remove all console.log statements

4. Poor API Architecture

Severity: High

Specific Architectural Issues:

1. God Controllers (700+ lines):

// src/endpoints/fapi/v1/order/orderController.ts (719 lines)
export class OrderController extends Controller {
  // Handles EVERYTHING: order creation, cancellation, retrieval, replacement
  // Example of mixed concerns (lines 509-621):
  @Get('/allOrders')
  public async getAllOrders(
    @Queries() query: GetAllOrdersQueryParams,
    @Request() req: express.Request,
  ): Promise<OrderResponse[]> {
    // 112 lines of business logic directly in controller
    // Redis operations, data transformation, error handling all mixed
  }
}

// src/endpoints/fapi/v1/market/marketController.ts (416 lines)
// Contains complex aggregation logic directly in controller methods

2. Mixed Controller Styles:

// TSOA style with decorators:
// src/endpoints/fapi/v1/order/orderController.ts
@Route('/fapi/v1')
@Tags('Futures Order')
export class OrderController extends Controller {
  @Post('/order')
  @OperationId('create-order')
  public async createOrder() {}
}

// Express style handlers:
// src/endpoints/api/order.ts
router.post('/v3/order', authenticateJWT(), async (req, res) => {
  // Direct route handler
});

3. Business Logic in Controllers:

// src/endpoints/fapi/v1/market/marketController.ts:318-414
@Get('/aggTrades')
public async getAggregateTrades() {
  // 96 lines of complex aggregation logic
  // Should be in a service layer
  const aggregatedResult = result.reduce((aggregator: any, trade: any) => {
    // Complex business logic...
  });
}

4. No Service Layer - Direct Redis Access:

// src/endpoints/fapi/v1/order/orderController.ts:147
const order = fold(await rdb.bookOrderStatus(
  RKEY_BOOK_ORDERS(body.symbol, req.keyAccountId),
  orderId,
));
// Controllers directly access Redis, no abstraction

5. Inconsistent API Versioning:

• /api/v3/* - One version scheme
• /fapi/v1/* - Different version scheme
• /fapi/v3/* - Mixed versions under same prefix
• /sapi/v1/* - Yet another prefix

6. No Clear Domain Boundaries:

// Order logic scattered across:
// - src/endpoints/api/order.ts
// - src/endpoints/fapi/v1/order/orderController.ts
// - src/endpoints/fapi/v1/order/index.ts
// - src/endpoints/fapi/v3/order/* (if exists)

Impact:

• Difficult to test (need to mock Redis in controllers)
• Hard to maintain and extend
• Inconsistent API behavior
• Poor scalability

Recommendation:

• Implement service layer pattern
• Use repository pattern for data access
• Standardize on one controller style
• Create clear domain boundaries

5. Type Safety Issues

Severity: Medium

Specific Type Safety Problems:

1. Extensive Use of any Type:

// src/endpoints/fapi/v1/market/marketController.ts:369
// @ts-expect-error need to check what is going on here
fromId = '(' + _.last(rawOrders)![0].toString('utf8');

// src/endpoints/fapi/v1/order/orderController.ts:420, 649, 794
(batch as any).bookProcessActivity(
  RKEY_BOOK_ACTIVITY(body.symbol),
  Date.now(),
);

// src/endpoints/api/order.ts:354
// @ts-expect-error need to update interface
batch.bookProcessActivity(RKEY_BOOK_ACTIVITY(body.symbol), Date.now());

2. Missing Return Types:

// src/utils/requestValidation.ts
export const fold = (hgetallResponse: any) => { // Returns any
  const result: any = {};
  // No type safety on returned object
}

// src/expectors.ts - Multiple functions without return types
export const expectString = (data: unknown) => {
  // No return type specified
}

3. Untyped Error Objects:

// src/endpoints/fapi/v1/order/orderController.ts:563
.catch((err) => reject(new Error(JSON.stringify(err))));
// Error details lost in stringification

// src/endpoints/api/order.ts:611-613
.catch((err) => reject(new Error(JSON.stringify(err))));

4. Type Assertions Without Validation:

// src/endpoints/fapi/v1/market/marketController.ts:377
const aggregatedResult = result.reduce((aggregator: any, trade: any) => {
  // Using any instead of proper types
});

5. Missing Interface Definitions:

// No type definition for Redis responses
const order = fold(await rdb.bookOrderStatus(...)); // order is any
order['quantity'] // No autocomplete or type checking
order['executedQty'] // Could be undefined

6. Ignored TypeScript Errors:

// Multiple @ts-expect-error comments instead of fixing types
// src/endpoints/api/order.ts:354
// src/endpoints/fapi/v1/market/marketController.ts:369

More Locations:

• src/expectors.ts - All validation functions lack proper return types
• src/formatters.ts - Functions operate on any types
• src/endpoints/fapi/v1/account/accountV1Controller.ts - Multiple any usages

Impact:

• Loss of TypeScript benefits
• Runtime errors from undefined properties
• Poor IDE support and autocomplete
• Difficult refactoring

Recommendation:

• Enable TypeScript strict mode
• Replace all any with proper types
• Add return types to all functions
• Create interfaces for all data structures

6. Poor Error Handling

Severity: Medium

Specific Error Handling Issues:

1. Generic Error Messages Without Context:

// src/endpoints/fapi/v1/order/orderController.ts:563
.catch((err) => reject(new Error(JSON.stringify(err))));

// src/endpoints/api/order.ts:611-613
.catch((err) => reject(new Error(JSON.stringify(err))));

// Multiple places with generic "something went wrong"
throw new Error('something went wrong...');

2. Console Logging Instead of Proper Error Handling:

// src/endpoints/fapi/v1/order/orderController.ts:629
console.log('hello'); // Random debug log

// src/endpoints/fapi/v3/token/tokenController.ts:95
console.error('something went wrong when validating the signature:', err);
// Error logged but not properly handled

// src/endpoints/api/order.ts:560
console.error('error on discard', err);

3. Inconsistent Error Response Formats:

// Format 1 - ExpectError
throw new ExpectError(['field'], 'error message');

// Format 2 - Plain Error
throw new Error('something went wrong...');

// Format 3 - Custom error response
res.status(400).json({ message: 'Invalid request' });

// No standardized error structure across API

4. Missing Try-Catch in Async Operations:

// src/endpoints/api/order.ts - Multiple async operations without proper error handling
const order = fold(await rdb.bookOrderStatus(...));
// No try-catch, potential unhandled rejection

5. Weak Input Validation:

// src/expectors.ts
export const expectString = (data: unknown): string => {
  if (typeof data !== 'string') {
    throw new ExpectError(['value'], 'must be a string');
  }
  return data;
}
// No length validation, no pattern validation, no sanitization

6. Error Details Exposed to Client:

// src/app.ts:66-90
if (process.env.NODE_ENV !== 'production') {
  res.status(status).json({
    message,
    fields,
    stack: err.stack, // Internal stack trace exposed
  });
}

More Issues:

• No validation for array bounds
• No null/undefined checks before property access
• Stream errors not properly propagated
• Promise rejections in callbacks not handled

Impact:

• API crashes from unhandled errors
• Poor debugging experience
• Security vulnerabilities from exposed internals
• Inconsistent client error handling

Recommendation:

• Implement global error handler with consistent format
• Add try-catch to all async operations
• Create custom error classes for different scenarios
• Use validation library (Joi, Zod) for input validation

Additional Issues

7. Incomplete Test Coverage

Only 4 test files in entire API:

• src/endpoints/fapi/v1/order/order.test.ts
• src/endpoints/fapi/v1/order/placeOrders.test.ts
• src/endpoints/fapi/v1/market/market.test.ts
• src/endpoints/fapi/v3/token/token.test.ts

Missing Tests for:

• Error handling scenarios
• Authentication middleware (src/middleware/auth.ts)
• Complex business logic in controllers
• Redis operations and edge cases
• Input validation
• Security vulnerabilities

8. Hardcoded Values and TODOs

Extensive TODO Comments:

// src/endpoints/fapi/v1/order/orderController.ts:174-189
workingType: 'CONTRACT_PRICE', // TODO,
positionSide: 'SHORT', // TODO,
closePosition: false, // TODO
time: Date.now(), // TODO
updateTime: Date.now(), // TODO
activatePrice: '0', // TODO
avgPrice: '0', // TODO
orderId: 0, // TODO
goodTillDate: Date.now(), // TODO
origType: 'TRAILING_STOP_MARKET', // TODO
priceMatch: '00', // TODO
priceProtect: false, // TODO
priceRate: '0', // TODO
reduceOnly: false, // TODO
stopPrice: '0', // TODO

Unimplemented Endpoints:

// src/endpoints/api/order.ts:967-1001
router.get('/v3/allOrders', authenticateJWT(), async (req, res) => {
  // Returns hardcoded empty array instead of real data
  res.json([]);
});

// src/endpoints/api/market.ts:319-322
// TODO: calculate taker buy and sell volumes
takerBuyBaseAssetVolume: '0',
takerBuyQuoteAssetVolume: '0',

9. Performance and Scalability Issues

Inefficient Redis Operations:

// src/endpoints/fapi/v1/order/orderController.ts:638-713
// Processing orders one by one in stream
stream.on('data', (currentOrders: string[]) => {
  // Individual Redis calls for each order
  for (const orderId of currentOrders) {
    pipeline.hgetall(RKEY_BOOK_ORDERS_METADATA(query.symbol, orderId));
  }
});

No Caching Strategy:

• No caching for frequently accessed data
• Every request hits Redis directly
• No CDN or response caching headers

Missing Optimizations:

• No pagination limits enforced
• Large result sets loaded entirely into memory
• No connection pooling configuration visible

10. Missing Monitoring and Observability

No Structured Logging:

• Using console.log and console.error
• No correlation IDs for request tracking
• No performance metrics collection

No Health Checks:

• No /health endpoint
• No Redis connection monitoring
• No dependency health validation

Recommendations Priority

1. Immediate (Security Critical):

   • Fix JWT secret configuration
   • Add rate limiting
   • Implement input sanitization

2. Short-term (1-2 weeks):

   • Add TypeScript strict mode
   • Extract duplicated code
   • Implement error handling framework

3. Medium-term (1 month):

   • Refactor to service layer architecture
   • Add comprehensive type definitions
   • Implement Redis type safety

4. Long-term (2-3 months):

   • Standardize API versioning
   • Add comprehensive test suite
   • Implement monitoring and logging

Conclusion

The codebase exhibits significant technical debt that poses risks to reliability, security, and maintainability. Immediate action is required on security vulnerabilities, while a structured refactoring plan should address architectural and code quality issues.