New strategies

[Faouzijedidi1]

Description

Update multiple strategy
Multiple Fixes.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Syntax Error There is a stray pm2 token appended to a block comment terminator, which will likely break TypeScript compilation. Remove the unexpected text to restore valid syntax. /** * Cancel leftover orders for a given exchange, symbol, and strategyKey. */pm2 private async cancelAllOrders( exchange: ccxt.Exchange, Concurrency Bug The volume strategy uses setInterval with an async loop without any re-entrancy/locking, so overlapping executions can occur if one cycle takes longer than the interval, potentially placing/canceling orders concurrently and causing inconsistent state. Consider adding a per-strategy mutex/flag and skipping if a run is already in progress, or using a self-scheduling setTimeout pattern. const loop = async () => { if (tradesExecuted >= numTrades) { this.logger.log( `Volume strategy [${strategyKey}] completed after ${numTrades} trades. ` + `Total PnL: ${totalPnL >= 0 ? '+' : ''}${totalPnL.toFixed(8)}` ); const inst = this.strategyInstances.get(strategyKey); if (inst?.intervalId) clearInterval(inst.intervalId); this.strategyInstances.delete(strategyKey); await this.strategyInstanceRepository.update( { strategyKey }, { status: 'stopped', updatedAt: new Date() }, ); return; } const tradeNumber = tradesExecuted + 1; try { await Promise.all([ this.cancelAllOrders(ex1, symbol, strategyKey), this.cancelAllOrders(ex2, symbol, strategyKey), ]); const [bal1, bal2] = await Promise.all([ ex1.fetchBalance(), ex2.fetchBalance(), ]); const [base, quote] = symbol.split('/'); const ex1Base = Number(bal1.free[base] ?? 0); const ex1Quote = Number(bal1.free[quote] ?? 0); const ex2Base = Number(bal2.free[base] ?? 0); const ex2Quote = Number(bal2.free[quote] ?? 0); // Use ex1 orderbook as global reference const book1 = await ex1.fetchOrderBook(symbol); const bid1 = book1.bids[0]?.[0]; const ask1 = book1.asks[0]?.[0]; if (!bid1 || !ask1) throw new Error('Empty orderbook on ex1'); const globalMid = (bid1 + ask1) / 2; const priceForCapacity = globalMid > 0 ? globalMid : (minPrice || 1e-12); const makerSide: 'buy' | 'sell' = postOnlySide; const takerSide: 'buy' | 'sell' = makerSide === 'buy' ? 'sell' : 'buy'; // Capacity if ex1 = maker, ex2 = taker const capacity1 = Math.min( makerSide === 'buy' ? ex1Quote / priceForCapacity : ex1Base, makerSide === 'buy' ? ex2Base : ex2Quote / priceForCapacity, ); // Capacity if ex2 = maker, ex1 = taker const capacity2 = Math.min( makerSide === 'buy' ? ex2Quote / priceForCapacity : ex2Base, makerSide === 'buy' ? ex1Base : ex1Quote / priceForCapacity, ); let makerEx = capacity1 >= capacity2 ? ex1 : ex2; let takerEx = capacity1 >= capacity2 ? ex2 : ex1; let maxCapacity = capacity1 >= capacity2 ? capacity1 : capacity2; if (!maxCapacity || maxCapacity <= 0 || maxCapacity < minAmt) { this.logger.warn( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: insufficient combined balance. ` + `ex1Base=${ex1Base.toFixed(8)} ex1Quote=${ex1Quote.toFixed(8)} ex2Base=${ex2Base.toFixed( 8, )} ex2Quote=${ex2Quote.toFixed(8)}`, ); return; } // Maker orderbook (reuse ex1 book if makerEx is ex1) const makerBook = makerEx === ex1 ? book1 : await makerEx.fetchOrderBook(symbol); const makerBid = makerBook.bids[0]?.[0]; const makerAsk = makerBook.asks[0]?.[0]; if (!makerBid || !makerAsk) throw new Error(`Empty orderbook on maker exchange ${makerEx.id}`); const mid = (makerBid + makerAsk) / 2; const makerRawPrice = Math.max(mid, minPrice || 1e-12); const makerPrice = priceToPrec(makerRawPrice); let rawAmt = Math.min(baseTradeAmount, maxCapacity) * 0.99; let amount = amtToPrec(rawAmt); if (!amount || amount <= 0 || amount < minAmt) { this.logger.warn( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: computed amount too small. ` + `amount=${amount} minAmt=${minAmt} maxCapacity=${maxCapacity}`, ); return; } // Helper to recompute capacity after switching maker/taker const computeCapacity = (me: ccxt.Exchange, te: ccxt.Exchange) => { const bMaker = me === ex1 ? bal1 : bal2; const bTaker = te === ex1 ? bal1 : bal2; const mBase = Number(bMaker.free[base] ?? 0); const mQuote = Number(bMaker.free[quote] ?? 0); const tBase = Number(bTaker.free[base] ?? 0); const tQuote = Number(bTaker.free[quote] ?? 0); const mMax = makerSide === 'buy' ? mQuote / priceForCapacity : mBase; const tMax = makerSide === 'buy' ? tBase : tQuote / priceForCapacity; return Math.min(mMax, tMax); }; let makerOrder: any; let takerOrder: any; let lastTakerPrice: number | undefined; // ----------------------- // MAKER ORDER (with fallback) // ----------------------- try { makerOrder = await makerEx.createOrder( symbol, 'limit', makerSide, amount, makerPrice, { postOnly: true }, ); this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `Maker order placed: ${makerOrder.id}` ); } catch (e: any) { this.logger.warn( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: maker failed on ${makerEx.id}: ${e.message}. ` + `Switching maker/taker and recomputing amount.`, ); // Swap roles const newMaker = makerEx === ex1 ? ex2 : ex1; const newTaker = makerEx === ex1 ? ex1 : ex2; makerEx = newMaker; takerEx = newTaker; const cap = computeCapacity(makerEx, takerEx); if (!cap || cap <= 0 || cap < minAmt) { throw new Error( `Alternate maker/taker insufficient balance for Trade ${tradeNumber} / ${numTrades}`, ); } const newAmt = amtToPrec(Math.min(baseTradeAmount, cap)); if (!newAmt || newAmt <= 0 || newAmt < minAmt) { throw new Error( `Alternate maker amount below minAmt for Trade ${tradeNumber} / ${numTrades}`, ); } amount = newAmt; makerOrder = await makerEx.createOrder( symbol, 'limit', makerSide, amount, makerPrice, { postOnly: true }, ); this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `Alternate maker order placed: ${makerOrder.id}` ); } // ----------------------- // 30ms SAFETY DELAY // ----------------------- await new Promise(resolve => setTimeout(resolve, 30)); // ----------------------- // TAKER ORDER (LIMIT + IOC, with fallback) // ----------------------- try { const takerLimitPrice = makerPrice; lastTakerPrice = takerLimitPrice + (takerLimitPrice*0.0000001); takerOrder = await takerEx.createOrder( symbol, 'limit', takerSide, amount, takerLimitPrice, { timeInForce: 'IOC' }, ); this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `Taker order placed: ${takerOrder.id}` ); } catch (e: any) { this.logger.warn( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: taker failed on ${takerEx.id}: ${e.message}. ` + `Switching taker and recomputing amount.`, ); const newTaker = takerEx === ex1 ? ex2 : ex1; takerEx = newTaker; const cap = computeCapacity(makerEx, takerEx); if (!cap || cap <= 0 || cap < minAmt) { try { if (makerOrder?.id) await makerEx.cancelOrder(makerOrder.id, symbol); } catch (_) {} throw new Error( `Alternate taker insufficient balance for Trade ${tradeNumber} / ${numTrades}`, ); } const newAmt = amtToPrec(Math.min(baseTradeAmount, cap)); if (!newAmt || newAmt <= 0 || newAmt < minAmt) { try { if (makerOrder?.id) await makerEx.cancelOrder(makerOrder.id, symbol); } catch (_) {} throw new Error( `Alternate taker amount below minAmt for Trade ${tradeNumber} / ${numTrades}`, ); } amount = newAmt; const takerLimitPrice = makerPrice; lastTakerPrice = takerLimitPrice; takerOrder = await takerEx.createOrder( symbol, 'limit', takerSide, amount, takerLimitPrice, { timeInForce: 'IOC' }, ); this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `Alternate taker order placed: ${takerOrder.id}` ); } // ----------------------- // LOG + FILL STATUS + PNL TRACKING // ----------------------- await new Promise(resolve => setTimeout(resolve, 200)); // Brief delay for order status update const [makerRes, takerRes] = await Promise.all([ makerEx.fetchOrder(makerOrder.id, symbol), takerEx.fetchOrder(takerOrder.id, symbol), ]); const makerFilled = makerRes.filled ?? 0; const takerFilled = takerRes.filled ?? 0; const makerAvgPrice = makerRes.average ?? makerPrice; const takerAvgPrice = takerRes.average ?? (lastTakerPrice ?? makerPrice); // Calculate PnL let tradePnL = 0; if (makerFilled > 0 && takerFilled > 0) { const filledAmount = Math.min(makerFilled, takerFilled); if (makerSide === 'buy') { // We bought at makerAvgPrice and sold at takerAvgPrice tradePnL = (takerAvgPrice - makerAvgPrice) * filledAmount; } else { // We sold at makerAvgPrice and bought at takerAvgPrice tradePnL = (makerAvgPrice - takerAvgPrice) * filledAmount; } totalPnL += tradePnL; } this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `Maker ${makerSide.toUpperCase()} ${amount} @ ${makerPrice} on ${makerEx.id} ` + `status=${makerRes.status} filled=${makerFilled}/${amount} avgPrice=${makerAvgPrice}`, ); this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `Taker ${takerSide.toUpperCase()} ${amount} @ ${lastTakerPrice ?? 'N/A'} on ${takerEx.id} ` + `status=${takerRes.status} filled=${takerFilled}/${amount} avgPrice=${takerAvgPrice}`, ); this.logger.log( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: ` + `PnL: ${tradePnL >= 0 ? '+' : ''}${tradePnL.toFixed(8)} ${quote} | ` + `Cumulative: ${totalPnL >= 0 ? '+' : ''}${totalPnL.toFixed(8)} ${quote}` ); tradesExecuted++; } catch (err: any) { this.logger.error( `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: Error in trade cycle: ${err.message}`, ); } }; const intervalId = setInterval(loop, Math.max(baseIntervalTime, 1) * 1000); this.strategyInstances.set(strategyKey, { isRunning: true, intervalId }); this.logger.log( `Volume strategy [${strategyKey}] started on ${exchangeName} for ${symbol}. ` + `postOnlySide=${postOnlySide} numTrades=${numTrades}`, ); Flaky Tests The test reads the entered amount via textContent() on what appears to be an input (total_input), which typically requires inputValue() (or reading the value attribute). This can make the assertion unreliable and cause intermittent failures depending on DOM structure. test('create buy/sell market order', async ({ page }) => { let isDialogOpen = await page .locator('#order_confirm_modal') .evaluate((dialog) => dialog.classList.contains('modal-open')); expect(isDialogOpen).toBeFalsy(); const amount = '1234.2346'; // Click buy tab await page.getByTestId('type_buy').click(); // Enter amount await page.getByTestId('total_input').click(); await page.keyboard.type(amount); // Click buy action await page.getByTestId('confirm_order').click(); let payAmount = await page.getByTestId('order_confirm_pay_amount').textContent(); let actualAmount = await page.getByTestId('total_input').textContent(); expect(payAmount).toContain(actualAmount); // Confirm order await page.getByTestId('confirm_order_button').click(); // Close await page.getByTestId('close_order_modal').click(); await page.getByTestId('total_input').fill(''); // Click sell tab await page.getByTestId('type_sell').click(); // Enter amount await page.getByTestId('total_input').click(); await page.keyboard.type(amount); // Click sell action await page.getByTestId('confirm_order').click(); payAmount = await page.getByTestId('order_confirm_pay_amount').textContent(); actualAmount = await page.getByTestId('total_input').textContent(); expect(payAmount).toContain(actualAmount);

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove stray invalid token The stray pm2 token after the block comment will break TypeScript compilation. Remove it so the method declaration parses correctly. server/src/modules/strategy/strategy.service.ts [1510-1517] /** * Cancel leftover orders for a given exchange, symbol, and strategyKey. - */pm2 + */ private async cancelAllOrders( exchange: ccxt.Exchange, pair: string, strategyKey: string, ) { Suggestion importance[1-10]: 10 __ Why: The stray pm2 after the block comment (*/pm2) is invalid TypeScript and will break compilation of cancelAllOrders. Removing it is a clear, high-impact correctness fix.	High
	Prevent overlapping trade cycles setInterval will trigger a new async loop() even if the previous one is still running, causing overlapping cancels/orders and inconsistent tradesExecuted/PnL. Add an inFlight guard (or switch to recursive setTimeout) to ensure only one cycle runs at a time. server/src/modules/strategy/strategy.service.ts [434-741] +let inFlight = false; + const loop = async () => { - if (tradesExecuted >= numTrades) { - this.logger.log( - `Volume strategy [${strategyKey}] completed after ${numTrades} trades. ` + - `Total PnL: ${totalPnL >= 0 ? '+' : ''}${totalPnL.toFixed(8)}` - ); - const inst = this.strategyInstances.get(strategyKey); - if (inst?.intervalId) clearInterval(inst.intervalId); - this.strategyInstances.delete(strategyKey); - await this.strategyInstanceRepository.update( - { strategyKey }, - { status: 'stopped', updatedAt: new Date() }, - ); - return; - } + if (inFlight) return; + inFlight = true; + try { + if (tradesExecuted >= numTrades) { + this.logger.log( + `Volume strategy [${strategyKey}] completed after ${numTrades} trades. ` + + `Total PnL: ${totalPnL >= 0 ? '+' : ''}${totalPnL.toFixed(8)}` + ); + const inst = this.strategyInstances.get(strategyKey); + if (inst?.intervalId) clearInterval(inst.intervalId); + this.strategyInstances.delete(strategyKey); + await this.strategyInstanceRepository.update( + { strategyKey }, + { status: 'stopped', updatedAt: new Date() }, + ); + return; + } - const tradeNumber = tradesExecuted + 1; + const tradeNumber = tradesExecuted + 1; - try { - await Promise.all([ - this.cancelAllOrders(ex1, symbol, strategyKey), - this.cancelAllOrders(ex2, symbol, strategyKey), - ]); - ... - tradesExecuted++; - } catch (err: any) { - this.logger.error( - `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: Error in trade cycle: ${err.message}`, - ); + try { + await Promise.all([ + this.cancelAllOrders(ex1, symbol, strategyKey), + this.cancelAllOrders(ex2, symbol, strategyKey), + ]); + ... + tradesExecuted++; + } catch (err: any) { + this.logger.error( + `[${strategyKey}] Trade ${tradeNumber} / ${numTrades}: Error in trade cycle: ${err.message}`, + ); + } + } finally { + inFlight = false; } }; const intervalId = setInterval(loop, Math.max(baseIntervalTime, 1) * 1000); this.strategyInstances.set(strategyKey, { isRunning: true, intervalId }); Suggestion importance[1-10]: 8 __ Why: Using setInterval with an async loop() can overlap executions if a cycle takes longer than the interval, which can cause duplicated cancels/orders and inconsistent tradesExecuted/totalPnL. An inFlight guard (or switching to recursive setTimeout) meaningfully improves runtime correctness.	Medium
	Fix limit slippage direction The slippage direction is inverted for a limit “execution” style order: a buy should place higher (more aggressive) and a sell should place lower, otherwise orders may not fill and the strategy appears “stuck”. Flip the multipliers so the limit price moves toward execution, not away from it. server/src/modules/strategy/time-indicator.service.ts [188-191] const bps = params.slippageBps ?? 10; const entryPriceRaw = - side === 'buy' ? last * (1 - bps / 10_000) : last * (1 + bps / 10_000); + side === 'buy' ? last * (1 + bps / 10_000) : last * (1 - bps / 10_000); const entryPrice = pricePrec(entryPriceRaw); Suggestion importance[1-10]: 6 __ Why: Current pricing makes buy limits less aggressive (last * (1 - bps)), and sell limits more aggressive (last * (1 + bps)), which can lead to non-filling orders when the intent is “execute with slippage”. Flipping the multipliers is a reasonable functional fix given executeLimitTrade() is used for entry.	Low
	Use correct exchange precision Precision is being derived from ex1 even when makerEx/takerEx is ex2, which can produce invalid price/amount formats and failed orders on the active exchange. Compute precision using the exchange you actually place the order on (especially for makerPrice and amount after role swaps). server/src/modules/strategy/strategy.service.ts [391-520] -const priceToPrec = (p: number) => Number(ex1.priceToPrecision(symbol, p)); -const amtToPrec = (a: number) => Number(ex1.amountToPrecision(symbol, a)); +const priceToPrec = (ex: ccxt.Exchange, p: number) => + Number(ex.priceToPrecision(symbol, p)); +const amtToPrec = (ex: ccxt.Exchange, a: number) => + Number(ex.amountToPrecision(symbol, a)); ... const makerRawPrice = Math.max(mid, minPrice || 1e-12); -const makerPrice = priceToPrec(makerRawPrice); +const makerPrice = priceToPrec(makerEx, makerRawPrice); ... let rawAmt = Math.min(baseTradeAmount, maxCapacity) * 0.99; -let amount = amtToPrec(rawAmt); +let amount = amtToPrec(makerEx, rawAmt); Suggestion importance[1-10]: 3 __ Why: The suggestion is generally sound, but here ex1 and ex2 are both initialized for the same exchangeName, so priceToPrecision/amountToPrecision should be identical across accounts and not a likely failure source. Still, using makerEx for precision would make the code more robust if the logic ever expands to different exchanges.	Low
Security	Avoid storing private keys Persisting dto stores signerPk1/signerPk2 in the database, which is a severe credential leak. Save a sanitized copy of parameters (omit or mask private keys) while still using the original dto in-memory for execution. server/src/modules/strategy/dex-volume.strategy.service.ts [54-64] +const { signerPk1: _pk1, signerPk2: _pk2, ...safeParams } = dto; + await this.strategyRepo.save( this.strategyRepo.create({ strategyKey, userId, clientId, strategyType: 'dexVolume', - parameters: dto, + parameters: safeParams, status: 'running', startPrice: 0, }), ); Suggestion importance[1-10]: 9 __ Why: Saving parameters: dto persists signerPk1/signerPk2 to the DB, which is a severe secret/credential leak. Persisting a sanitized object (omitting private keys) materially improves security without affecting runtime execution.	High