refactor: centralize CCXT instance caching with ccxtInstanceManager#52
refactor: centralize CCXT instance caching with ccxtInstanceManager#52suenot wants to merge 3 commits into
Conversation
Adding CLAUDE.md with task information for AI processing. This file will be removed when the task is complete. Issue: #32
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new experimental validation script and refactors components and the browser provider to centralize CCXT instance management via ccxtInstanceManager. Components adopt async flows with fallback to direct ccxt instantiation. The provider removes local caches and delegates instance/market handling, cache operations, and cleanup to ccxtInstanceManager. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Component (TestChartWidget/TestTimeframes)
participant M as ccxtInstanceManager
participant CCXT as ccxt (direct fallback)
UI->>M: getExchangeInstanceForMarket(exchange, creds, sandbox)
alt Manager success
M-->>UI: exchangeInstance
UI->>UI: Proceed with tests (fetch, timeframes, etc.)
else Manager failure
UI->>CCXT: new ccxt.binance(...)\n(fallback)
CCXT-->>UI: exchangeInstance (direct)
UI->>UI: Proceed with limited/fallback tests
end
sequenceDiagram
autonumber
participant P as ccxtBrowserProvider
participant M as ccxtInstanceManager
participant Pro as getCCXTPro (fallback)
participant Ex as Exchange Instance
rect rgb(240,245,255)
note over P: getCCXTInstance(exchange, opts)
alt isPro
P->>Pro: request CCXT Pro
Pro-->>P: ExchangeClass / instance
P-->>Ex: return Pro instance (wrapped as needed)
else regular CCXT
P->>M: getExchangeInstanceForMarket(exchange, opts)
M-->>P: instance
P-->>Ex: return instance
end
end
rect rgb(245,255,245)
note over P: getMarketsForExchange / getSymbolsForExchange
P->>M: getExchangeInstanceForMarket(exchange, metadataOpts)
M-->>P: instance (for markets/capabilities)
P->>P: derive markets/symbols
end
rect rgb(255,248,235)
note over P: cache ops
P->>M: clear/cleanup/getStats/invalidate
M-->>P: delegated results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
…aching - Refactor TestChartWidget.tsx to use ccxtInstanceManager instead of direct CCXT instantiation - Refactor TestTimeframes.tsx to use ccxtInstanceManager with fallback for comparison - Refactor ccxtBrowserProvider to delegate to ccxtInstanceManager for unified caching - Remove duplicate caching mechanisms and static cache from ccxtBrowserProvider - Remove duplicate cleanup intervals as ccxtInstanceManager handles this centrally - Keep fallback CCXT Pro logic until ccxtInstanceManager supports Pro version - Add validation test script in experiments/ to verify refactoring Fixes #32 - ccxtInstanceManager is now used consistently throughout the codebase 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
suenot
changed the title
🤖 Solution Draft LogThis log file contains the complete execution trace of the AI solution draft process. 📎 Log file uploaded as GitHub Gist (406KB) Log automatically attached by solve.mjs with --attach-logs option |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/store/providers/ccxtBrowserProvider.ts (3)
228-238: Consider making the metadata accountId configurable or more descriptive.The hardcoded
'metadata-account'(line 231) works but might be unclear to maintainers. Consider using a constant or a more descriptive identifier like'__metadata_public__'to signal its special purpose.+// At top of file +const METADATA_ACCOUNT_ID = '__metadata_public__'; + async getMarketsForExchange(exchange: string): Promise<string[]> { try { const exchangeInstance = await ccxtInstanceManager.getExchangeInstanceForMarket( exchange, - 'metadata-account', + METADATA_ACCOUNT_ID, {
83-149: Add error handling for ccxtInstanceManager failures in public methods.The public methods
getTradingInstance(lines 83-107),getMetadataInstance(lines 112-128), andgetWebSocketInstance(lines 133-149) callCCXTBrowserProviderImpl.getCCXTInstancebut don't add any error handling beyond what getCCXTInstance provides. If ccxtInstanceManager throws unexpected errors or the fallback path fails, callers won't get helpful context about which method failed. Consider wrapping the getCCXTInstance calls with try-catch blocks that add method-specific context.Example for getTradingInstance:
async getTradingInstance( userId: string, accountId: string, exchangeId: string, marketType: string, ccxtType: 'regular' | 'pro', credentials: { apiKey: string; secret: string; password?: string; sandbox?: boolean; } ): Promise<any> { + try { const config = createCCXTInstanceConfig( this.provider.id, userId, accountId, exchangeId, marketType, ccxtType, credentials ); return CCXTBrowserProviderImpl.getCCXTInstance(config); + } catch (error) { + console.error(`❌ [CCXTBrowser] Failed to get trading instance for ${exchangeId}:${marketType}`, error); + throw error; + } }
36-64: Document CCXT Pro support timeline and enrich TODO
Replace the placeholder comment at line 35 with a reference to the GitHub issue tracking CCXT Pro support and an expected ETA. ccxtInstanceManager already wraps all exchanges withwrapExchangeWithLogger(see lines 167 & 239), so no additional logger handling is needed here.experiments/test-refactoring.js (4)
21-30: LGTM! Export validation is clear and straightforward.The string matching approach is suitable for this validation script. The exact pattern match ensures the singleton export is present as expected.
For slightly more flexibility, consider using a regex pattern that tolerates whitespace variations:
-if (ccxtInstanceManagerContent.includes('export const ccxtInstanceManager = new CCXTInstanceManager()')) { +if (/export\s+const\s+ccxtInstanceManager\s*=\s*new\s+CCXTInstanceManager\(\s*\)/.test(ccxtInstanceManagerContent)) {
49-64: LGTM! Consistent validation pattern for TestTimeframes.The test follows the same thorough validation approach as Test 2, checking both import and usage.
Consider extracting a helper function to reduce duplication between Tests 2 and 3:
function validateComponentUsage(componentName, filePath, content) { console.log(`\nChecking ${componentName}...`); if (content.includes('import { ccxtInstanceManager }')) { console.log(`✅ ${componentName} imports ccxtInstanceManager`); } else { console.log(`❌ ${componentName} does not import ccxtInstanceManager`); } if (content.includes('ccxtInstanceManager.getExchangeInstanceForMarket')) { console.log(`✅ ${componentName} uses ccxtInstanceManager.getExchangeInstanceForMarket`); } else { console.log(`❌ ${componentName} does not use ccxtInstanceManager`); } }
98-103: Consider more specific validation for cleanup interval removal.The current logic uses a broad pattern that could yield false positives if
setIntervalis used for other purposes in the file. The OR condition with a Russian comment string is brittle and language-specific.Consider a more specific pattern that checks for the cleanup-related
setInterval:-if (!ccxtBrowserProviderContent.includes('setInterval(() => {') || - ccxtBrowserProviderContent.includes('Cleanup теперь управляется централизованно')) { +// Check for absence of the old cleanup interval pattern +const hasOldCleanupInterval = /setInterval\s*\(\s*\(\s*\)\s*=>\s*\{[^}]*cleanup[^}]*\}/i.test(ccxtBrowserProviderContent); +if (!hasOldCleanupInterval) { console.log('✅ Duplicate cleanup interval removed or replaced'); } else { console.log('❌ Duplicate cleanup interval still exists'); }
105-134: Validate direct instantiation detection, but fallback counting is unreliable.The detection of direct CCXT instantiations using
/new ccxt\./gis appropriate. However, the fallback counting logic has issues:
- The regex
/Fallback|fallback|CCXT Pro.*fallback/giis too broad and will match comments, strings, and unrelated text- The comparison
foundDirectInstantiations <= expectedFallbackInstantiationsassumes perfect correlation between fallback mentions and legitimate direct instantiations- This could produce false positives if "fallback" appears in comments or documentation
Consider a more robust approach:
let foundDirectInstantiations = 0; -let expectedFallbackInstantiations = 0; filesToCheck.forEach(filePath => { const content = fs.readFileSync(path.join(__dirname, filePath), 'utf8'); const directInstantiations = (content.match(/new ccxt\./g) || []).length; - const fallbackInstantiations = (content.match(/Fallback|fallback|CCXT Pro.*fallback/gi) || []).length; foundDirectInstantiations += directInstantiations; - expectedFallbackInstantiations += fallbackInstantiations; if (directInstantiations > 0) { - console.log(` - ${path.basename(filePath)}: ${directInstantiations} direct instantiation(s) found`); + // Extract context around each instantiation to verify it's a fallback + const lines = content.split('\n'); + lines.forEach((line, idx) => { + if (/new ccxt\./.test(line)) { + const context = lines.slice(Math.max(0, idx - 2), idx + 3).join('\n'); + const isFallback = /fallback|catch|error/i.test(context); + console.log(` - ${path.basename(filePath)}:${idx + 1}: ${isFallback ? '(fallback)' : '⚠️ non-fallback'}`); + } + }); } }); -if (foundDirectInstantiations <= expectedFallbackInstantiations) { +if (foundDirectInstantiations === 0) { - console.log('✅ Direct CCXT instantiations limited to fallback cases only'); + console.log('✅ No direct CCXT instantiations found'); } else { - console.log(`❌ Found ${foundDirectInstantiations} direct instantiations, expected only ${expectedFallbackInstantiations} fallback cases`); + console.log(`⚠️ Found ${foundDirectInstantiations} direct instantiation(s) - review above to verify they are fallbacks`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
experiments/test-refactoring.js(1 hunks)src/components/TestChartWidget.tsx(4 hunks)src/components/TestTimeframes.tsx(2 hunks)src/store/providers/ccxtBrowserProvider.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/TestChartWidget.tsx (1)
src/store/utils/ccxtInstanceManager.ts (1)
ccxtInstanceManager(326-326)
src/components/TestTimeframes.tsx (2)
src/store/utils/ccxtUtils.ts (1)
getCCXT(11-17)src/store/utils/ccxtInstanceManager.ts (1)
ccxtInstanceManager(326-326)
src/store/providers/ccxtBrowserProvider.ts (5)
src/types/dataProviders.ts (1)
CCXTBrowserProvider(159-162)src/store/utils/ccxtProviderUtils.ts (1)
CCXTInstanceConfig(8-19)src/store/utils/ccxtUtils.ts (1)
getCCXTPro(22-30)src/utils/requestLogger.ts (1)
wrapExchangeWithLogger(243-291)src/store/utils/ccxtInstanceManager.ts (1)
ccxtInstanceManager(326-326)
🔇 Additional comments (16)
src/components/TestChartWidget.tsx (5)
37-70: LGTM! Clean async integration with appropriate fallback.The refactor correctly delegates instance creation to
ccxtInstanceManagerwith proper error handling and a sensible fallback to direct instantiation. The async flow is properly awaited inrunAllTests(line 118).
149-150: Good: Lambda wrapper ensures proper async invocation.The lambda
() => testCCXT()correctly handles the async call without blocking the render cycle.
37-70: Verify the broad error handling doesn't hide manager-specific issues.The catch block at line 58 captures all errors from ccxtInstanceManager without distinguishing between recoverable issues (e.g., network timeout) and configuration errors (e.g., invalid exchange ID). In a test component, consider logging the error type or checking
managerError instanceofspecific error classes to provide more diagnostic information.
118-118: LGTM!Correctly awaits the async
testCCXT()call to ensure sequential execution in the test suite.
149-150: LGTM!The lambda wrapper
() => testCCXT()properly invokes the async function without causing React warnings about async event handlers.src/components/TestTimeframes.tsx (2)
14-74: LGTM! Async test flow with comprehensive error handling.The refactor correctly integrates
ccxtInstanceManagerwith proper try/catch blocks and fallback logic. The nested error handling (lines 37-50) ensures graceful degradation when the manager fails.
58-68: No async issue: getTimeframesForExchange is synchronous. The function’s signature ((exchange: string): Timeframe[]) and its helpergetTimeframesFromCCXTboth return arrays directly, so there’s no Promise-based behavior—no refactor needed.src/store/providers/ccxtBrowserProvider.ts (4)
305-337: LGTM! Clean delegation to centralized manager.All cache operations correctly delegate to
ccxtInstanceManager, preserving the public API while removing duplicate caching logic.
305-337: LGTM!Cache management methods correctly delegate to ccxtInstanceManager. The
invalidateCachemethod appropriately handles both targeted (by exchangeId) and full cache clearing scenarios.
66-77: LGTM!Regular CCXT instances are correctly delegated to ccxtInstanceManager with all necessary configuration parameters (exchangeId, accountId, credentials, marketType).
232-236: Normalize empty credential strings toundefined. PassingapiKey: ''andsecret: ''(src/store/providers/ccxtBrowserProvider.ts lines 232–236) can differ from omitting them in CCXT’s validation logic. Convert empty strings toundefined(for example, inccxtInstanceManageruseapiKey: accountConfig.apiKey || undefined, and likewise forsecret) to ensure blank credentials are treated as “not set.”experiments/test-refactoring.js (5)
1-20: LGTM! Clean setup with proper ESM support.The script correctly uses ESM imports and includes the necessary
__dirnameworkaround for ES modules. The header documentation clearly describes the validation purpose.
32-47: LGTM! Comprehensive validation of TestChartWidget integration.The test correctly validates both the import declaration and actual usage of
ccxtInstanceManager.getExchangeInstanceForMarket, ensuring the component has fully adopted the centralized instance manager.
66-87: LGTM! Thorough validation of provider delegation.The test correctly validates that
ccxtBrowserProviderproperly delegates toccxtInstanceManagerfor both instance retrieval and cache management operations.
89-96: LGTM! Clear validation of duplicate cache removal.The check for
private static instancesCacheensures the duplicate caching structure has been removed fromccxtBrowserProvider.
136-143: LGTM! Clear and informative summary.The refactoring summary provides a concise overview of the changes validated by the script, making it easy to understand what was accomplished.
| // Test Binance instance creation using ccxtInstanceManager | ||
| try { | ||
| const binance = await ccxtInstanceManager.getExchangeInstanceForMarket( | ||
| 'binance', | ||
| 'test-account', | ||
| { | ||
| apiKey: 'test', | ||
| secret: 'test', | ||
| sandbox: true | ||
| }, | ||
| 'spot' | ||
| ); | ||
| addResult(`✅ Binance instance created via ccxtInstanceManager: ${binance.id}`); | ||
| } catch (managerError) { | ||
| addResult(`❌ ccxtInstanceManager error: ${managerError}`); | ||
| // Fallback to direct instantiation for comparison | ||
| const binance = new ccxt.binance(); | ||
| addResult(`⚠️ Fallback - Binance instance created directly: ${binance.id}`); | ||
| } |
There was a problem hiding this comment.
Fallback creates inconsistent instance without credentials.
The fallback at line 61 instantiates ccxt.binance() without credentials, while the manager call (lines 47-56) passes apiKey, secret, and sandbox configuration. This inconsistency means the fallback won't actually test the same scenario and could mask configuration or authentication issues in ccxtInstanceManager.
Apply this diff to align the fallback with the manager configuration:
} catch (managerError) {
addResult(`❌ ccxtInstanceManager error: ${managerError}`);
// Fallback to direct instantiation for comparison
- const binance = new ccxt.binance();
+ const binance = new ccxt.binance({
+ apiKey: 'test',
+ secret: 'test',
+ sandbox: true
+ });
addResult(`⚠️ Fallback - Binance instance created directly: ${binance.id}`);
}📝 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.
| // Test Binance instance creation using ccxtInstanceManager | |
| try { | |
| const binance = await ccxtInstanceManager.getExchangeInstanceForMarket( | |
| 'binance', | |
| 'test-account', | |
| { | |
| apiKey: 'test', | |
| secret: 'test', | |
| sandbox: true | |
| }, | |
| 'spot' | |
| ); | |
| addResult(`✅ Binance instance created via ccxtInstanceManager: ${binance.id}`); | |
| } catch (managerError) { | |
| addResult(`❌ ccxtInstanceManager error: ${managerError}`); | |
| // Fallback to direct instantiation for comparison | |
| const binance = new ccxt.binance(); | |
| addResult(`⚠️ Fallback - Binance instance created directly: ${binance.id}`); | |
| } | |
| // Test Binance instance creation using ccxtInstanceManager | |
| try { | |
| const binance = await ccxtInstanceManager.getExchangeInstanceForMarket( | |
| 'binance', | |
| 'test-account', | |
| { | |
| apiKey: 'test', | |
| secret: 'test', | |
| sandbox: true | |
| }, | |
| 'spot' | |
| ); | |
| addResult(`✅ Binance instance created via ccxtInstanceManager: ${binance.id}`); | |
| } catch (managerError) { | |
| addResult(`❌ ccxtInstanceManager error: ${managerError}`); | |
| // Fallback to direct instantiation for comparison | |
| const binance = new ccxt.binance({ | |
| apiKey: 'test', | |
| secret: 'test', | |
| sandbox: true | |
| }); | |
| addResult(`⚠️ Fallback - Binance instance created directly: ${binance.id}`); | |
| } |
🤖 Prompt for AI Agents
In src/components/TestChartWidget.tsx around lines 45 to 63, the fallback
instantiates ccxt.binance() without credentials which is inconsistent with the
earlier manager call that passes apiKey, secret and sandbox; change the fallback
to create the Binance instance with the same credentials and sandbox
configuration used above (pass the same apiKey and secret when constructing the
ccxt.binance instance and enable sandbox/testnet mode if the ccxt exchange
exposes a setSandboxMode or equivalent option) so the fallback tests the same
authentication/configuration scenario as ccxtInstanceManager.
| const binanceInstance = await ccxtInstanceManager.getExchangeInstanceForMarket( | ||
| 'binance', | ||
| 'test-timeframes-account', | ||
| { | ||
| apiKey: 'test', | ||
| secret: 'test', | ||
| sandbox: true | ||
| }, | ||
| 'spot' | ||
| ); | ||
| console.log('📊 [TestTimeframes] Binance timeframes via ccxtInstanceManager:', binanceInstance.timeframes); | ||
| testResults.binance_direct = binanceInstance.timeframes; | ||
| } catch (error) { | ||
| console.error('❌ [TestTimeframes] Error creating Binance instance via ccxtInstanceManager:', error); | ||
| testResults.binance_direct = { error: error.message }; | ||
|
|
||
| // Fallback to direct instantiation for comparison | ||
| try { | ||
| const binanceInstanceFallback = new ccxt.binance(); | ||
| console.log('⚠️ [TestTimeframes] Fallback - Binance timeframes:', binanceInstanceFallback.timeframes); | ||
| testResults.binance_direct = binanceInstanceFallback.timeframes; | ||
| } catch (fallbackError) { | ||
| console.error('❌ [TestTimeframes] Fallback also failed:', fallbackError); | ||
| testResults.binance_direct = { error: fallbackError.message }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback inconsistency and result overwriting issue.
Two problems here:
-
The fallback at line 43 creates a Binance instance without the credentials that were passed to ccxtInstanceManager (lines 28-32), similar to the issue in TestChartWidget.tsx. This makes the fallback test a different scenario.
-
Lines 36 and 45 both assign to
testResults.binance_direct, so if the manager succeeds, that result is immediately overwritten by the fallback attempt. The fallback block should only assign on fallback success.
Apply this diff:
const binanceInstance = await ccxtInstanceManager.getExchangeInstanceForMarket(
'binance',
'test-timeframes-account',
{
apiKey: 'test',
secret: 'test',
sandbox: true
},
'spot'
);
console.log('📊 [TestTimeframes] Binance timeframes via ccxtInstanceManager:', binanceInstance.timeframes);
testResults.binance_direct = binanceInstance.timeframes;
} catch (error) {
console.error('❌ [TestTimeframes] Error creating Binance instance via ccxtInstanceManager:', error);
- testResults.binance_direct = { error: error.message };
// Fallback to direct instantiation for comparison
try {
- const binanceInstanceFallback = new ccxt.binance();
+ const binanceInstanceFallback = new ccxt.binance({
+ apiKey: 'test',
+ secret: 'test',
+ sandbox: true
+ });
console.log('⚠️ [TestTimeframes] Fallback - Binance timeframes:', binanceInstanceFallback.timeframes);
testResults.binance_direct = binanceInstanceFallback.timeframes;
} catch (fallbackError) {
console.error('❌ [TestTimeframes] Fallback also failed:', fallbackError);
testResults.binance_direct = { error: fallbackError.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.
| const binanceInstance = await ccxtInstanceManager.getExchangeInstanceForMarket( | |
| 'binance', | |
| 'test-timeframes-account', | |
| { | |
| apiKey: 'test', | |
| secret: 'test', | |
| sandbox: true | |
| }, | |
| 'spot' | |
| ); | |
| console.log('📊 [TestTimeframes] Binance timeframes via ccxtInstanceManager:', binanceInstance.timeframes); | |
| testResults.binance_direct = binanceInstance.timeframes; | |
| } catch (error) { | |
| console.error('❌ [TestTimeframes] Error creating Binance instance via ccxtInstanceManager:', error); | |
| testResults.binance_direct = { error: error.message }; | |
| // Fallback to direct instantiation for comparison | |
| try { | |
| const binanceInstanceFallback = new ccxt.binance(); | |
| console.log('⚠️ [TestTimeframes] Fallback - Binance timeframes:', binanceInstanceFallback.timeframes); | |
| testResults.binance_direct = binanceInstanceFallback.timeframes; | |
| } catch (fallbackError) { | |
| console.error('❌ [TestTimeframes] Fallback also failed:', fallbackError); | |
| testResults.binance_direct = { error: fallbackError.message }; | |
| } | |
| } | |
| const binanceInstance = await ccxtInstanceManager.getExchangeInstanceForMarket( | |
| 'binance', | |
| 'test-timeframes-account', | |
| { | |
| apiKey: 'test', | |
| secret: 'test', | |
| sandbox: true | |
| }, | |
| 'spot' | |
| ); | |
| console.log('📊 [TestTimeframes] Binance timeframes via ccxtInstanceManager:', binanceInstance.timeframes); | |
| testResults.binance_direct = binanceInstance.timeframes; | |
| } catch (error) { | |
| console.error('❌ [TestTimeframes] Error creating Binance instance via ccxtInstanceManager:', error); | |
| // Fallback to direct instantiation for comparison | |
| try { | |
| const binanceInstanceFallback = new ccxt.binance({ | |
| apiKey: 'test', | |
| secret: 'test', | |
| sandbox: true | |
| }); | |
| console.log('⚠️ [TestTimeframes] Fallback - Binance timeframes:', binanceInstanceFallback.timeframes); | |
| testResults.binance_direct = binanceInstanceFallback.timeframes; | |
| } catch (fallbackError) { | |
| console.error('❌ [TestTimeframes] Fallback also failed:', fallbackError); | |
| testResults.binance_direct = { error: fallbackError.message }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/TestTimeframes.tsx around lines 25 to 50, the fallback logic
is creating a Binance instance without the same credentials/sandbox passed to
ccxtInstanceManager and is overwriting testResults.binance_direct even when the
manager succeeded; fix by (1) in the catch fallback, instantiate the
ccxt.binance with the same options used in the manager call (apiKey, secret,
sandbox) so it tests the same scenario, and (2) only assign
testResults.binance_direct inside the fallback success branch (do not overwrite
the value set by the manager success path); keep the original manager assignment
unchanged and ensure fallback assignment happens only if the manager call failed
and the fallback succeeds.
| private static async getCCXTInstance(config: CCXTInstanceConfig): Promise<any> { | ||
| const instanceKey = CCXTBrowserProviderImpl.createInstanceKey(config); | ||
| const cached = CCXTBrowserProviderImpl.instancesCache.get(instanceKey); | ||
|
|
||
| // Проверяем кэш | ||
| if (cached && CCXTBrowserProviderImpl.isInstanceValid(cached)) { | ||
| cached.lastAccess = Date.now(); | ||
| console.log(`📋 [CCXTBrowser] Using cached instance: ${instanceKey}`); | ||
| return cached.instance; | ||
| } | ||
|
|
||
| // Создаем новый instance | ||
| console.log(`🔄 [CCXTBrowser] Creating new instance: ${instanceKey}`); | ||
|
|
||
| let ccxtLib; | ||
| let ExchangeClass; | ||
| console.log(`🔄 [CCXTBrowser] Delegating to ccxtInstanceManager for ${config.exchangeId}:${config.marketType}`); | ||
|
|
||
| // Для Pro версии CCXT пока используем существующую логику | ||
| // TODO: Расширить ccxtInstanceManager для поддержки Pro версии | ||
| if (config.ccxtType === 'pro') { | ||
| ccxtLib = getCCXTPro(); | ||
| console.warn(`⚠️ [CCXTBrowser] CCXT Pro not yet supported by ccxtInstanceManager, using fallback`); | ||
|
|
||
| const ccxtLib = getCCXTPro(); | ||
| if (!ccxtLib) { | ||
| throw new Error('CCXT Pro not available'); | ||
| } | ||
| ExchangeClass = ccxtLib[config.exchangeId]; | ||
| } else { | ||
| ccxtLib = getCCXT(); | ||
| if (!ccxtLib) { | ||
| throw new Error('CCXT not available'); | ||
| } | ||
| ExchangeClass = ccxtLib[config.exchangeId]; | ||
| } | ||
|
|
||
| if (!ExchangeClass) { | ||
| throw new Error(`Exchange ${config.exchangeId} not found in CCXT${config.ccxtType === 'pro' ? ' Pro' : ''}`); | ||
| } | ||
| const ExchangeClass = ccxtLib[config.exchangeId]; | ||
| if (!ExchangeClass) { | ||
| throw new Error(`Exchange ${config.exchangeId} not found in CCXT Pro`); | ||
| } | ||
|
|
||
| // Маппинг типов рынков для разных бирж | ||
| let defaultType = config.marketType; | ||
| if (config.exchangeId === 'bybit') { | ||
| const bybitCategoryMap: Record<string, string> = { | ||
| 'spot': 'spot', | ||
| 'futures': 'linear', | ||
| 'swap': 'linear', | ||
| 'margin': 'spot', | ||
| 'options': 'option' | ||
| const instanceConfig = { | ||
| sandbox: config.sandbox || false, | ||
| apiKey: config.apiKey, | ||
| secret: config.secret, | ||
| password: config.password, | ||
| enableRateLimit: true, | ||
| defaultType: config.marketType, | ||
| }; | ||
| defaultType = bybitCategoryMap[config.marketType] || config.marketType; | ||
| console.log(`🔍 [CCXTBrowser] Bybit mapping: ${config.marketType} -> ${defaultType}`); | ||
| } | ||
|
|
||
| const instanceConfig = { | ||
| sandbox: config.sandbox || false, | ||
| apiKey: config.apiKey, | ||
| secret: config.secret, | ||
| password: config.password, | ||
| enableRateLimit: true, | ||
| defaultType: defaultType, | ||
| }; | ||
|
|
||
| console.log(`🔍 [CCXTBrowser] Creating ${config.exchangeId} ${config.ccxtType} instance:`, { | ||
| providerId: config.providerId, | ||
| userId: config.userId, | ||
| accountId: config.accountId, | ||
| sandbox: instanceConfig.sandbox, | ||
| apiKey: instanceConfig.apiKey ? 'SET' : 'NOT_SET', | ||
| secret: instanceConfig.secret ? 'SET' : 'NOT_SET', | ||
| defaultType: instanceConfig.defaultType, | ||
| marketType: config.marketType, | ||
| ccxtType: config.ccxtType | ||
| }); | ||
|
|
||
| const exchangeInstance = new ExchangeClass(instanceConfig); | ||
|
|
||
| // Wrap with request logger | ||
| const loggedInstance = wrapExchangeWithLogger( | ||
| exchangeInstance, | ||
| config.exchangeId, | ||
| `${config.userId}:${config.accountId}` | ||
| ); | ||
| const exchangeInstance = new ExchangeClass(instanceConfig); | ||
| return wrapExchangeWithLogger( | ||
| exchangeInstance, | ||
| config.exchangeId, | ||
| `${config.userId}:${config.accountId}` | ||
| ); | ||
| } | ||
|
|
||
| // Загружаем markets с кэшированием | ||
| await CCXTBrowserProviderImpl.loadMarketsWithCache( | ||
| loggedInstance, | ||
| config.exchangeId, | ||
| config.sandbox || false, | ||
| // Используем ccxtInstanceManager для regular CCXT | ||
| return ccxtInstanceManager.getExchangeInstanceForMarket( | ||
| config.exchangeId, | ||
| config.accountId, | ||
| { | ||
| apiKey: config.apiKey, | ||
| secret: config.secret, | ||
| password: config.password, | ||
| sandbox: config.sandbox | ||
| }, | ||
| config.marketType | ||
| ); | ||
|
|
||
| // Кэшируем instance | ||
| const cachedInstance: CachedCCXTInstance = { | ||
| instance: loggedInstance, | ||
| config: { ...config }, | ||
| lastAccess: Date.now(), | ||
| marketsLoaded: true | ||
| }; | ||
|
|
||
| CCXTBrowserProviderImpl.instancesCache.set(instanceKey, cachedInstance); | ||
| console.log(`✅ [CCXTBrowser] Cached new instance: ${instanceKey}, total cache size: ${CCXTBrowserProviderImpl.instancesCache.size}`); | ||
|
|
||
| return loggedInstance; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify Pro instance lifecycle management.
The Pro fallback path (lines 36-64) creates an unmanaged instance that bypasses ccxtInstanceManager. This means Pro instances won't benefit from centralized caching, cleanup, or stats tracking.
Consider tracking this technical debt explicitly until ccxtInstanceManager supports Pro:
🏁 Script executed:
#!/bin/bash
# Description: Check for other Pro instance creation patterns that might need similar treatment
# Search for getCCXTPro usage across the codebase
rg -n --type=ts -C3 'getCCXTPro\(\)'Length of output: 1735
Centralize CCXT Pro instance management.
Pro instantiations in src/store/providers/ccxtBrowserProvider.ts, src/store/utils/ccxtAccountManager.ts, and src/store/actions/fetchingActions.ts bypass ccxtInstanceManager, forfeiting centralized caching, cleanup, and stats. Extend ccxtInstanceManager to support Pro instances or explicitly track these fallbacks as technical debt.
🤖 Prompt for AI Agents
In src/store/providers/ccxtBrowserProvider.ts around lines 31 to 78, the CCXT
Pro branch instantiates Pro exchanges directly, bypassing ccxtInstanceManager
and losing centralized caching/cleanup/stats; update the code by extending
ccxtInstanceManager to accept a flag or separate API for Pro instances (e.g.,
support ccxtType:'pro'), move the Pro instantiation logic into that manager
(including creation, caching keyed by exchangeId+accountId+marketType, proper
teardown hooks and stats tracking), and then replace the direct fallback here
with a call to ccxtInstanceManager.getExchangeInstanceForMarket(..., { ccxtType:
'pro', ...config }); if immediate manager changes are not feasible, add a TODO
and register these fallback instances in a central registry with caching/cleanup
and emit a tech-debt marker in the manager for future consolidation.
| // Используем ccxtInstanceManager для получения metadata instance | ||
| const exchangeInstance = await ccxtInstanceManager.getExchangeInstanceForMarket( | ||
| exchange, | ||
| 'metadata-account', | ||
| { | ||
| apiKey: '', | ||
| secret: '', | ||
| sandbox: false | ||
| }, | ||
| 'spot' | ||
| ); |
There was a problem hiding this comment.
Hardcoded 'spot' marketType may not reflect all exchange capabilities.
Line 237 always passes 'spot' when obtaining the metadata instance. For exchanges that don't support spot trading or have different primary markets (e.g., derivatives-only exchanges), this could fail or return incomplete market information. Consider passing marketType as a parameter or using a capability detection approach.
Consider this refactor to make market type configurable:
- async getMarketsForExchange(exchange: string): Promise<string[]> {
+ async getMarketsForExchange(exchange: string, preferredMarketType: string = 'spot'): Promise<string[]> {
try {
// Используем ccxtInstanceManager для получения metadata instance
const exchangeInstance = await ccxtInstanceManager.getExchangeInstanceForMarket(
exchange,
'metadata-account',
{
apiKey: '',
secret: '',
sandbox: false
},
- 'spot'
+ preferredMarketType
);🤖 Prompt for AI Agents
In src/store/providers/ccxtBrowserProvider.ts around lines 228 to 238, the code
always passes the hardcoded string 'spot' to
ccxtInstanceManager.getExchangeInstanceForMarket which can fail or return
incomplete metadata for exchanges that are derivatives-only or have other
primary markets; change this call to accept a configurable marketType (add a
parameter to the function or read it from the exchange config), or detect
supported market types first (query exchange capabilities or attempt a fallback
list like ['spot','futures','swap']) and pass the appropriate marketType into
getExchangeInstanceForMarket so the metadata instance reflects the exchange's
actual market capabilities.
1 participant
Summary
This PR refactors the codebase to use
ccxtInstanceManagereverywhere for centralized CCXT instance caching, eliminating duplicate caching mechanisms.Changes Made
🔧 Core Refactoring
new ccxt.binance()withccxtInstanceManager.getExchangeInstanceForMarket()🗑️ Duplicate Code Removal
instancesCacheandmarketsCachefromCCXTBrowserProviderImplcreateInstanceKey,isInstanceValid,loadMarketsWithCache🔄 Delegation Implementation
getCCXTInstance()now delegates toccxtInstanceManager.getExchangeInstanceForMarket()clearCache,cleanup,getCacheStats) delegate to ccxtInstanceManager🧪 Testing & Validation
experiments/test-refactoring.jsvalidation script that confirms:Technical Details
Before: Multiple independent caching systems
ccxtInstanceManager.ts- Unused centralized cacheccxtBrowserProvider.ts- Own static cache with duplicate logicAfter: Single unified caching system
ccxtInstanceManager- Central authority for all CCXT instancesccxtBrowserProvider- Thin wrapper delegating to ccxtInstanceManagerCompatibility Notes
DebugCCXTCache.tsxcontinues to work unchanged via delegated methodsTest Results
Fixes #32
🤖 Generated with Claude Code
Summary by CodeRabbit