feat: restore api search #159
Conversation
WalkthroughAdds a Redis-backed TokenCacheRepository, wires it into services and the API, implements a token search endpoint, and introduces a token-list-updater app with fetching, caching, tests, Docker, and CI/Jest/ESLint/Nx scaffolding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as Fastify API
participant Repo as TokenCacheRepository
Note over API,Repo: Token search request flow
Client->>API: GET /:chainId/tokens/search/:searchParam
API->>Repo: getTokenListBySearchParam(chainId, searchParam)
Repo-->>API: Token[] or []
API-->>Client: 200 OK (Token[]) / 500 on error
sequenceDiagram
autonumber
participant UpdaterApp as token-list-updater
participant Factory as getTokenCacheRepository()
participant Updater as TokenListUpdater
participant DS as getTokensByChainName()
participant CG as CoinGecko
participant Repo as TokenCacheRepository
Note over UpdaterApp: Startup
UpdaterApp->>Factory: obtain repository
loop per supported chain (staggered)
UpdaterApp->>Updater: new TokenListUpdater({chainId, repo, delay})
Updater->>Updater: start() (doForever loop, 6h cadence)
alt update cycle
Updater->>DS: getTokensByChainName(chainName, chainId)
DS->>CG: HTTP GET tokens.<platform>.json
CG-->>DS: tokens JSON
DS-->>Updater: Token[]
Updater->>Repo: initTokenList(chainId, tokens)
end
end
Note over UpdaterApp: Shutdown
UpdaterApp->>Updater: stop() for all
Updater-->>UpdaterApp: stopped or timeout (30s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
libs/repositories/jest.config.ts (1)
9-11: Same transformIgnorePatterns refinement as notification-producerMirror the cross‑platform regex and preferably hoist to the shared preset to avoid drift.
- transformIgnorePatterns: [ - 'node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)/)', - ], + transformIgnorePatterns: [ + 'node_modules[\\\\/](?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)[\\\\/])', + ],libs/services/jest.config.ts (1)
9-11: Keep the regex portable and deduplicatedAdopt the same cross‑platform pattern and consider centralizing in ../../jest.preset.js.
- transformIgnorePatterns: [ - 'node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)/)', - ], + transformIgnorePatterns: [ + 'node_modules[\\\\/](?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)[\\\\/])', + ],
🧹 Nitpick comments (31)
libs/repositories/src/repos/UsdRepository/UsdRepositoryCoingecko.test.ts (2)
101-109: Bound the gap on both sides and ensure monotonic timestampsEven with the corrected tolerance, the current check is less explicit than a min/max bound and doesn’t verify strictly increasing timestamps. Recommend asserting both lower and upper bounds and monotonicity.
@@ - expect( - Math.abs( - price.date.getTime() - previousPrice.date.getTime() - FIVE_MINUTES - ) - ).toBeLessThanOrEqual(FIVE_MINUTES * BUFFER_ERROR_TOLERANCE); // 5 min of error tolerance (we don't need to be super precise, but we also want to assert the points are kind of 5min apart) + const delta5m = price.date.getTime() - previousPrice.date.getTime(); + expect(delta5m).toBeGreaterThanOrEqual(FIVE_MINUTES * (1 - BUFFER_ERROR_TOLERANCE)); + expect(delta5m).toBeLessThanOrEqual(FIVE_MINUTES * (1 + BUFFER_ERROR_TOLERANCE)); + expect(price.date.getTime()).toBeGreaterThan(previousPrice.date.getTime()); @@ - expect( - Math.abs( - price.date.getTime() - previousPrice.date.getTime() - ONE_HOUR - ) - ).toBeLessThanOrEqual(ONE_HOUR * BUFFER_ERROR_TOLERANCE); // 1 hour of error tolerance (we don't need to be super precise, but we also want to assert the points are kind of 1 hour apart) + const delta1h = price.date.getTime() - previousPrice.date.getTime(); + expect(delta1h).toBeGreaterThanOrEqual(ONE_HOUR * (1 - BUFFER_ERROR_TOLERANCE)); + expect(delta1h).toBeLessThanOrEqual(ONE_HOUR * (1 + BUFFER_ERROR_TOLERANCE)); + expect(price.date.getTime()).toBeGreaterThan(previousPrice.date.getTime()); @@ - expect( - Math.abs( - price.date.getTime() - previousPrice.date.getTime() - ONE_DAY - ) - ).toBeLessThanOrEqual(ONE_DAY * BUFFER_ERROR_TOLERANCE); // 1 day of error tolerance (we don't need to be super precise, but we also want to assert the points are kind of 1 day apart) + const delta1d = price.date.getTime() - previousPrice.date.getTime(); + expect(delta1d).toBeGreaterThanOrEqual(ONE_DAY * (1 - BUFFER_ERROR_TOLERANCE)); + expect(delta1d).toBeLessThanOrEqual(ONE_DAY * (1 + BUFFER_ERROR_TOLERANCE)); + expect(price.date.getTime()).toBeGreaterThan(previousPrice.date.getTime());Also applies to: 167-174, 195-202
167-167: Fix copy-paste in comments: “5 minutes” vs hourly/dailyThese two inline comments still say “5 minutes” inside the hourly and daily tests.
- // Check the time difference between the two prices is around 5 minutes + // Check the time difference between the two prices is around 1 hour- // Check the time difference between the two prices is around 5 minutes + // Check the time difference between the two prices is around 1 dayAlso applies to: 195-195
apps/notification-producer/jest.config.ts (1)
9-11: Make transformIgnorePatterns cross-platform and centralize in jest.preset.js- transformIgnorePatterns: [ - 'node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)/)', - ], + transformIgnorePatterns: [ + 'node_modules[\\\\/](?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)[\\\\/])', + ],Remove these duplicate entries from libs/services/jest.config.ts, libs/repositories/jest.config.ts and apps/notification-producer/jest.config.ts and keep a single source of truth in ../../jest.preset.js.
libs/repositories/src/datasources/tokenSearch/index.ts (1)
1-1: Consider re‑exporting tokenSearch types to avoid deep importsRe‑exporting types here helps consumers stay on the barrel and reduces path churn.
export * from './tokenList'; +export * from './types';libs/repositories/src/index.ts (1)
7-7: Expose tokenSearch types in public barrelAdd a type-only re-export of
TokenFromAPIin libs/repositories/src/index.ts to let consumers import it from the barrel instead of deep paths:export * from './datasources/tokenSearch'; +export type { TokenFromAPI } from './datasources/tokenSearch/types';apps/api/src/main.ts (1)
21-22: Prefer structured logging for errorsUse the shared logger’s structured payload to preserve stack and avoid odd serialization.
- logger.warn('Token cache repository not initialized:', error); + logger.warn({ err: error }, 'Token cache repository not initialized');apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (1)
34-36: Reduce log noise for hot pathSearch endpoints can be high QPS; log at debug to avoid inflating info-level logs.
- fastify.log.info( + fastify.log.debug( `Token search for "${searchParam}" on chain ${chainId}: ${tokens.length} tokens found` );libs/services/src/factories.ts (1)
186-194: Memoize TokenCacheRepository to avoid multiple wrappers over the same clientMultiple call sites create separate instances. Make it a singleton.
-export function getTokenCacheRepository(): TokenCacheRepository { +let _tokenCacheRepo: TokenCacheRepository | undefined; +export function getTokenCacheRepository(): TokenCacheRepository { if (!redisClient) { throw new Error( 'Redis client is required for TokenCacheRepository. Please configure Redis.' ); } - - return new TokenCacheRepositoryRedis(redisClient); + if (!_tokenCacheRepo) { + _tokenCacheRepo = new TokenCacheRepositoryRedis(redisClient); + } + return _tokenCacheRepo; }Note: I’m intentionally not suggesting caching for SimulationRepository per past learning that the benefit isn’t worth it in this codebase.
libs/repositories/src/datasources/tokenSearch/tokenListSearch.ts (2)
5-7: Tighten the map typing for better safetyPrefer a chain-aware map type and conventional param naming.
Apply:
-export function tokenListSearch( - TOKEN_MAP: Record<string, TokenFromAPI[]>, +export function tokenListSearch( + tokenMap: Partial<Record<SupportedChainId, TokenFromAPI[]>>, chainId: SupportedChainId, searchParam: string ) { - const trimmedSearchParam = searchParam.trim().toLowerCase(); + const trimmedSearchParam = searchParam.trim().toLowerCase(); - return TOKEN_MAP[chainId].filter( + const list = tokenMap[chainId] ?? []; + return list.filter(
4-4: Add explicit return typeMakes intent clear and prevents accidental signature changes.
Apply:
-export function tokenListSearch( +export function tokenListSearch( tokenMap: Partial<Record<SupportedChainId, TokenFromAPI[]>>, chainId: SupportedChainId, searchParam: string -) +) : TokenFromAPI[]libs/repositories/src/datasources/tokenSearch/types.ts (1)
3-10: Confirm optionality of fields coming from CoinGeckoSome entries in the upstream list may miss
logoURI(and occasionally other fields). Either makelogoURIoptional here or normalize defaults at fetch time (see suggestion in getTokensByChain).Option A (type-level):
export type TokenFromAPI = { chainId: SupportedChainId; address: string; name: string; symbol: string; decimals: number; - logoURI: string; + logoURI?: string; };Option B (fetch normalization): keep type as-is and set
logoURI: ''if missing when mapping fetched data.libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (1)
10-10: Use a shared logger instead of console.log in library codeConsole usage in libs makes observability noisy and inconsistent. Consider injecting a logger or using a debug namespace.
Also applies to: 28-28
libs/repositories/src/datasources/tokenSearch/tokenListSearch.test.ts (2)
1-7: Align map typing with implementationIf you adopt
Partial<Record<SupportedChainId, TokenFromAPI[]>>, reflect it in tests for consistency.Apply:
- const mockTokenMap: Record<string, TokenFromAPI[]> = { + const mockTokenMap: Partial<Record<SupportedChainId, TokenFromAPI[]>> = {
239-239: Add tests for missing-chain safety and uppercase address searchCovers the new guard and ensures case-insensitive address matching with uppercase input.
Apply:
@@ it('should handle special characters in search param', () => { const result = tokenListSearch(mockTokenMap, SupportedChainId.MAINNET, 'x'); @@ ).toBe(true); }); + + it('returns empty array when chain is not present in the map', () => { + const result = tokenListSearch({}, SupportedChainId.MAINNET, 'uni'); + expect(result).toEqual([]); + }); + + it('matches uppercase address input (case insensitive)', () => { + const addr = '0xdac17f958d2ee523a2206206994597c13d831ec7'.toUpperCase(); + const result = tokenListSearch( + mockTokenMap, + SupportedChainId.MAINNET, + addr + ); + expect(result).toHaveLength(1); + expect(result[0].symbol).toBe('USDT'); + });apps/api/src/app/routes/__chainId/tokens/initTokenList/schemas.ts (2)
13-15: Add a description to the success schema for better API docs.Apply:
export const successSchema = { type: 'boolean', + description: 'True if tokens were initialized or already cached (idempotent).', } as const satisfies JSONSchema;
17-28: Consider including a stable error code in errorSchema.This helps clients branch on error types without parsing messages.
export const errorSchema = { type: 'object', required: ['message'], additionalProperties: false, properties: { message: { title: 'Message', description: 'Message describing the error.', type: 'string', }, + code: { + title: 'Code', + description: 'Stable error code (e.g., E_UNSUPPORTED_CHAIN, E_FETCH_FAILURE).', + type: 'string', + }, }, } as const satisfies JSONSchema;libs/repositories/src/datasources/tokenSearch/tokenList.test.ts (3)
18-20: Avoid leaking the mocked global.fetch across test files.Move the assignment into beforeAll/afterAll and restore it.
-const mockFetch = jest.fn(); -global.fetch = mockFetch; +const mockFetch = jest.fn(); +let realFetch: typeof global.fetch; + +beforeAll(() => { + // @ts-expect-error jest mock + realFetch = global.fetch; + // @ts-expect-error jest mock + global.fetch = mockFetch; +}); + +afterAll(() => { + // @ts-expect-error restore + global.fetch = realFetch; +}); beforeEach(() => { jest.clearAllMocks(); mockFetch.mockClear(); setTokenCacheRepository(mockTokenCacheRepository); Object.values(mockTokenCacheRepository).forEach((mock) => mock.mockReset()); });Also applies to: 43-50
127-136: Make the “invalid tokens array” assertion less brittle.Error string can change. Prefer a regex on the salient part.
- await expect(initTokenList(SupportedChainId.MAINNET)).rejects.toThrow( - 'Invalid token list format from https://tokens.coingecko.com/ethereum/all.json: missing or invalid tokens array' - ); + await expect(initTokenList(SupportedChainId.MAINNET)).rejects.toThrow( + /Invalid token list format/i + );
234-254: Optional: Add a test for unsupported chain IDs.This would exercise the “Chain X is not supported by CoinGecko” path and document behavior.
I can add a small test case if you want.
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
32-35: Consider bounding search results via an optional limit parameter.Returning unbounded arrays can be heavy. A non-breaking interface tweak:
- searchTokens( - chainId: SupportedChainId, - searchParam: string - ): Promise<TokenFromAPI[]>; + searchTokens( + chainId: SupportedChainId, + searchParam: string, + limit?: number + ): Promise<TokenFromAPI[]>;Implementations can default to, say, 50.
libs/repositories/src/datasources/tokenSearch/tokenList.ts (3)
33-45: Guard against redundant work under concurrency and use structured logging.
- Multiple concurrent initTokenList calls can race and double-fetch. It’s harmless but wasteful. Consider a lightweight distributed lock (e.g., Redis SET NX with a short TTL) around the fetch+cache block.
- Replace console.log with the shared logger to get levels, sampling, and traces.
47-61: Optional: Short-circuit empty searchParam to avoid accidental full-list returns.Even though the API route validates minLength: 3, defensive coding here prevents misuse from other call sites.
export async function getTokenListBySearchParam( chainId: SupportedChainId, searchParam: string, cacheRepository?: TokenCacheRepository ): Promise<TokenFromAPI[]> { const repository = cacheRepository || tokenCacheRepository; if (!repository) { throw new Error( 'TokenCacheRepository not configured. Please set it using setTokenCacheRepository()' ); } - return repository.searchTokens(chainId, searchParam); + const q = searchParam.trim(); + if (!q) return []; + return repository.searchTokens(chainId, q); }
15-31: Optional: Pass-through TTL parameter when initializing.If callers need custom TTLs, consider adding an optional ttlSeconds param and forwarding to repository.initTokenList.
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (3)
9-16: Namespace Redis keys to avoid cross-app collisions.Use an env-based prefix.
- private readonly keyPrefix = 'tokens:'; + private readonly keyPrefix = + `${process.env.REDIS_KEY_PREFIX ?? 'bff'}:tokens:`;
39-46: Log (warn) on JSON parse errors before clearing.Adds observability when cache content is corrupted.
try { return JSON.parse(serializedTokens) as TokenFromAPI[]; } catch (error) { - // If parsing fails, clear the corrupted data + // If parsing fails, clear the corrupted data + // TODO: replace with shared logger + console.warn('Clearing corrupted token cache for', chainId, error); await this.clearTokenList(chainId); return null; }
68-72: Minor robustness: use exists > 0.Future-proof if getKey ever returns multiple keys.
- const exists = await this.redisClient.exists(key); - return exists === 1; + const exists = await this.redisClient.exists(key); + return exists > 0;apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (5)
3-3: Deduplicate chainId schema by reusing SupportedChainIdSchema (and drop AllChainIds import).Avoids divergence between request/response schemas and tightens types.
-import { AllChainIds } from '@cowprotocol/shared'; @@ - chainId: { - title: 'Chain ID', - description: 'Blockchain network identifier.', - type: 'integer', - enum: AllChainIds, - }, + chainId: SupportedChainIdSchema,Also applies to: 28-33
57-61: Validate logoURI as a URI.Adds basic URL validation.
logoURI: { title: 'Logo URI', description: 'URI to the token logo.', type: 'string', + format: 'uri', },
25-25: Consider making logoURI optional.Some tokens may not have a logo; requiring it can reject valid results. If logos are guaranteed by your source, ignore. Otherwise, remove from required.
- required: ['chainId', 'address', 'name', 'symbol', 'decimals', 'logoURI'], + required: ['chainId', 'address', 'name', 'symbol', 'decimals'],
11-17: Relax searchParam and forbid slashes.Two-letter symbols (e.g., “OK”) are common; also guard path-param safety.
type: 'string', - minLength: 3, + minLength: 2, maxLength: 100, + pattern: '^[^/]+$',
41-49: Disallow empty name/symbol.Prevents empty strings leaking into responses without over-constraining.
name: { title: 'Name', description: 'Full name of the token.', type: 'string', + minLength: 1, }, symbol: { title: 'Symbol', description: 'Token symbol/ticker.', type: 'string', + minLength: 1, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
apps/api/src/app/inversify.config.ts(4 hunks)apps/api/src/app/routes/__chainId/tokens/initTokenList/index.ts(1 hunks)apps/api/src/app/routes/__chainId/tokens/initTokenList/schemas.ts(1 hunks)apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts(1 hunks)apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts(1 hunks)apps/api/src/main.ts(2 hunks)apps/notification-producer/jest.config.ts(1 hunks)libs/repositories/jest.config.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/index.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.test.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenListSearch.test.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenListSearch.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/types.ts(1 hunks)libs/repositories/src/index.ts(2 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/index.ts(1 hunks)libs/repositories/src/repos/UsdRepository/UsdRepositoryCoingecko.test.ts(1 hunks)libs/services/jest.config.ts(1 hunks)libs/services/src/factories.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-22T11:36:04.042Z
Learnt from: anxolin
PR: cowprotocol/bff#124
File: libs/services/src/factories.ts:124-126
Timestamp: 2025-04-22T11:36:04.042Z
Learning: Simulation repositories (SimulationRepositoryTenderly) don't require a caching layer like other repositories in the system, as the performance benefit isn't worth the implementation effort.
Applied to files:
libs/services/src/factories.ts
🧬 Code graph analysis (14)
apps/api/src/main.ts (2)
libs/services/src/factories.ts (1)
getTokenCacheRepository(186-194)libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
setTokenCacheRepository(9-13)
libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (1)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
apps/api/src/app/routes/__chainId/tokens/initTokenList/schemas.ts (2)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (6)
paramsSchema(5-19)successSchema(21-64)errorSchema(66-77)RouteSchema(79-79)SuccessSchema(80-80)ErrorSchema(81-81)apps/api/src/app/schemas.ts (1)
SupportedChainIdSchema(3-8)
libs/services/src/factories.ts (2)
libs/repositories/src/datasources/redis.ts (1)
redisClient(9-17)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-49)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (3)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-49)libs/repositories/src/datasources/redis.ts (1)
redisClient(9-17)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
libs/repositories/src/datasources/tokenSearch/tokenListSearch.test.ts (2)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)libs/repositories/src/datasources/tokenSearch/tokenListSearch.ts (1)
tokenListSearch(4-17)
libs/repositories/src/datasources/tokenSearch/tokenListSearch.ts (1)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
libs/repositories/src/datasources/tokenSearch/tokenList.ts (5)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-49)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (1)
initTokenList(18-27)libs/repositories/src/datasources/coingecko.ts (1)
SUPPORTED_COINGECKO_PLATFORMS(10-23)libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (1)
getTokensByChainName(36-41)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
apps/api/src/app/inversify.config.ts (2)
libs/services/src/factories.ts (1)
getTokenCacheRepository(186-194)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (2)
TokenCacheRepository(6-49)tokenCacheRepositorySymbol(4-4)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (2)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (3)
paramsSchema(5-19)successSchema(21-64)errorSchema(66-77)libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
getTokenListBySearchParam(47-61)
apps/api/src/app/routes/__chainId/tokens/initTokenList/index.ts (2)
apps/api/src/app/routes/__chainId/tokens/initTokenList/schemas.ts (6)
RouteSchema(30-30)SuccessSchema(31-31)ErrorSchema(32-32)paramsSchema(4-11)successSchema(13-15)errorSchema(17-28)libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
initTokenList(15-45)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (3)
apps/api/src/app/routes/__chainId/tokens/initTokenList/schemas.ts (6)
paramsSchema(4-11)successSchema(13-15)errorSchema(17-28)RouteSchema(30-30)SuccessSchema(31-31)ErrorSchema(32-32)apps/api/src/app/schemas.ts (1)
SupportedChainIdSchema(3-8)libs/shared/src/const.ts (1)
AllChainIds(57-57)
libs/repositories/src/datasources/tokenSearch/tokenList.test.ts (4)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-49)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)libs/repositories/src/datasources/tokenSearch/tokenList.ts (3)
setTokenCacheRepository(9-13)initTokenList(15-45)getTokenListBySearchParam(47-61)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (1)
initTokenList(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: docker (apps/twap)
- GitHub Check: docker (apps/notification-producer)
- GitHub Check: docker (apps/api)
- GitHub Check: docker (apps/telegram)
🔇 Additional comments (6)
libs/repositories/src/index.ts (1)
66-68: Approve TokenCacheRepository barrel export
- No deep imports of
TokenCacheRepositoryorTokenCacheRepositoryRedisdetected; downstream code correctly uses the barrel.- Nit: comment headers mix “Data-sources” and “Data sources”; consider normalizing in a follow-up.
libs/repositories/src/repos/TokenCacheRepository/index.ts (1)
1-2: No action required – symbol already exported via barrel
ThetokenCacheRepositorySymbolis declared inTokenCacheRepository.tsand re-exported by therepos/TokenCacheRepository/index.tsbarrel (and transitively bylibs/repositories/src/index.ts), so no additional export is needed.Likely an incorrect or invalid review comment.
apps/api/src/app/routes/__chainId/tokens/initTokenList/schemas.ts (1)
4-11: LGTM: params schema is tight and consistent with shared SupportedChainIdSchema.libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
4-5: Good DI symbol export.libs/repositories/src/datasources/tokenSearch/tokenList.ts (2)
9-13: Setter-based DI makes testing straightforward.
1-61: setTokenCacheRepository is correctly wired during app startup: call confirmed in apps/api/src/main.ts (line 18).
| const cacheRepository = getCacheRepository(); | ||
| const erc20Repository = getErc20Repository(cacheRepository); | ||
| const simulationRepository = getSimulationRepository(); | ||
| const tokenCacheRepository = getTokenCacheRepository(); |
There was a problem hiding this comment.
Guard token-cache DI binding; don’t crash when Redis is absent
getTokenCacheRepository() throws without Redis. Wrap creation, bind only if available, and keep startup consistent with main.ts’ graceful path.
- const tokenCacheRepository = getTokenCacheRepository();
+ let tokenCacheRepository: TokenCacheRepository | undefined;
+ try {
+ tokenCacheRepository = getTokenCacheRepository();
+ } catch (err) {
+ logger.warn('Token cache repository not initialized; proceeding without Redis', err);
+ }
...
- apiContainer
- .bind<TokenCacheRepository>(tokenCacheRepositorySymbol)
- .toConstantValue(tokenCacheRepository);
+ if (tokenCacheRepository) {
+ apiContainer
+ .bind<TokenCacheRepository>(tokenCacheRepositorySymbol)
+ .toConstantValue(tokenCacheRepository);
+ }Longer-term, consider sourcing the single instance from DI and calling setTokenCacheRepository() with that instance to avoid duplication and ensure consistent lifecycle.
Also applies to: 92-95
🤖 Prompt for AI Agents
In apps/api/src/app/inversify.config.ts around line 58 (and similarly lines
92-95), getTokenCacheRepository() can throw when Redis is unavailable; wrap the
repository creation in a try/catch and only perform the DI bind when creation
succeeds, logging or no-oping on failure to preserve the existing graceful
startup path used in main.ts; as a follow-up consider obtaining a single
token-cache instance via DI and calling setTokenCacheRepository(instance) so the
instance lifecycle is consistent and not duplicated.
| schema: { | ||
| params: paramsSchema, | ||
| response: { | ||
| '2XX': successSchema, | ||
| '404': errorSchema, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Describe 500 (and 400) responses in the route schema
Handler returns 500, but schema only defines 2XX and 404. Add 500; consider 400 for “unsupported chain” errors.
Apply:
schema: {
params: paramsSchema,
response: {
- '2XX': successSchema,
- '404': errorSchema,
+ 200: successSchema,
+ 400: errorSchema,
+ 500: errorSchema,
},
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/app/routes/__chainId/tokens/initTokenList/index.ts around lines
20 to 26, the route schema only declares '2XX' and '404' responses while the
handler can return 500 (and may return 400 for unsupported chain), so update the
schema.response to add a '500' entry pointing to an error schema (and add a
'400' entry if you return bad-request for unsupported chain) so those error
shapes are documented; ensure the added entries reference the existing
errorSchema (or a specific badRequestSchema if you have one) and update any
related types/tests as needed.
| try { | ||
| await initTokenList(chainId); | ||
|
|
||
| fastify.log.info(`Token list initalized for chain ${chainId}`); | ||
|
|
||
| reply.send(true); | ||
| } catch (error) { | ||
| fastify.log.error('Error searching tokens:', error); | ||
| reply.code(500).send({ | ||
| message: 'Internal server error while searching tokens', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix log/message text and map known errors to proper status codes
Aligns copy with route intent and sends 400 for unsupported chains.
Apply:
- try {
- await initTokenList(chainId);
-
- fastify.log.info(`Token list initalized for chain ${chainId}`);
-
- reply.send(true);
- } catch (error) {
- fastify.log.error('Error searching tokens:', error);
- reply.code(500).send({
- message: 'Internal server error while searching tokens',
- });
- }
+ try {
+ await initTokenList(chainId);
+ fastify.log.info(`Token list initialized for chain ${chainId}`);
+ reply.send(true);
+ } catch (error) {
+ const msg = (error as Error)?.message ?? 'Unknown error';
+ fastify.log.error('Error initializing token list:', error);
+ if (/not supported by CoinGecko/i.test(msg)) {
+ return reply.code(400).send({ message: msg });
+ }
+ reply.code(500).send({
+ message: 'Internal server error while initializing token list',
+ });
+ }📝 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 { | |
| await initTokenList(chainId); | |
| fastify.log.info(`Token list initalized for chain ${chainId}`); | |
| reply.send(true); | |
| } catch (error) { | |
| fastify.log.error('Error searching tokens:', error); | |
| reply.code(500).send({ | |
| message: 'Internal server error while searching tokens', | |
| }); | |
| try { | |
| await initTokenList(chainId); | |
| fastify.log.info(`Token list initialized for chain ${chainId}`); | |
| reply.send(true); | |
| } catch (error) { | |
| const msg = (error as Error)?.message ?? 'Unknown error'; | |
| fastify.log.error('Error initializing token list:', error); | |
| if (/not supported by CoinGecko/i.test(msg)) { | |
| return reply.code(400).send({ message: msg }); | |
| } | |
| reply.code(500).send({ | |
| message: 'Internal server error while initializing token list', | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/app/routes/__chainId/tokens/initTokenList/index.ts around lines
31 to 41, the catch block uses incorrect log/message text and always returns
500; update the log to reflect initialization (e.g., "Error initializing token
list for chain:") and change the response mapping so known unsupported-chain
errors return 400. Specifically, detect the unsupported-chain error (e.g.,
error.name === 'UnsupportedChainError' or instanceof UnsupportedChainError) and
reply.code(400).send({ message: 'Unsupported chain' }); otherwise log the error
and reply.code(500).send({ message: 'Internal server error while initializing
token list' }). Ensure the log includes the error object/details.
apps/api/src/main.ts
Outdated
| // Initialize token cache repository | ||
| try { | ||
| const tokenCacheRepository = getTokenCacheRepository(); | ||
| setTokenCacheRepository(tokenCacheRepository); | ||
| logger.info('Token cache repository initialized with Redis'); | ||
| } catch (error) { | ||
| logger.warn('Token cache repository not initialized:', error); | ||
| } |
There was a problem hiding this comment.
Startup handles missing Redis gracefully, but DI still hard-fails — align both paths
Main catches getTokenCacheRepository() failures and proceeds, but inversify.config.ts unconditionally constructs/binds the same repo and will throw if Redis isn’t configured. Result: app still crashes during container build even though this block tries to degrade gracefully.
Apply this change in apps/api/src/app/inversify.config.ts to make the binding optional and consistent with this file’s behavior:
- const tokenCacheRepository = getTokenCacheRepository();
+ let tokenCacheRepository: TokenCacheRepository | undefined;
+ try {
+ tokenCacheRepository = getTokenCacheRepository();
+ } catch (err) {
+ logger.warn('Token cache repository not initialized; proceeding without Redis', err);
+ }
...
- apiContainer
- .bind<TokenCacheRepository>(tokenCacheRepositorySymbol)
- .toConstantValue(tokenCacheRepository);
+ if (tokenCacheRepository) {
+ apiContainer
+ .bind<TokenCacheRepository>(tokenCacheRepositorySymbol)
+ .toConstantValue(tokenCacheRepository);
+ }Additionally consider unifying on a single source of truth (either DI or the module-level setter) to avoid double instantiation. I can draft that follow-up if you want.
📝 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.
| // Initialize token cache repository | |
| try { | |
| const tokenCacheRepository = getTokenCacheRepository(); | |
| setTokenCacheRepository(tokenCacheRepository); | |
| logger.info('Token cache repository initialized with Redis'); | |
| } catch (error) { | |
| logger.warn('Token cache repository not initialized:', error); | |
| } | |
| // apps/api/src/app/inversify.config.ts | |
| // Make the TokenCacheRepository binding optional, mirroring the behavior in main.ts | |
| let tokenCacheRepository: TokenCacheRepository | undefined; | |
| try { | |
| tokenCacheRepository = getTokenCacheRepository(); | |
| } catch (err) { | |
| logger.warn('Token cache repository not initialized; proceeding without Redis', err); | |
| } | |
| if (tokenCacheRepository) { | |
| apiContainer | |
| .bind<TokenCacheRepository>(tokenCacheRepositorySymbol) | |
| .toConstantValue(tokenCacheRepository); | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/app/inversify.config.ts (around lines 1-200), the container
unconditionally constructs and binds the Redis-backed token cache repository
which will throw if Redis isn’t configured; make the binding optional and
consistent with main.ts by wrapping the creation in a try/catch and only binding
the concrete Redis repository when construction succeeds, otherwise bind a safe
fallback (e.g., a no-op in-memory/null implementation or leave the token cache
binding absent and bind a nullable/optional token cache token). Also avoid
double-instantiation by resolving the module-level setter: if
getTokenCacheRepository() already created an instance, have the DI binding use
that instance (inject the existing instance or export a factory that returns the
module-level repository) so you don’t create two repositories.
| return data.tokens.map((token: TokenFromAPI) => ({ | ||
| ...token, | ||
| chainId, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize mapped fields and provide safe defaults
Do not trust upstream types blindly; map and coerce to our contract.
Apply:
- return data.tokens.map((token: TokenFromAPI) => ({
- ...token,
- chainId,
- }));
+ const { tokens } = data as { tokens: any[] };
+ return tokens.map((t: any): TokenFromAPI => ({
+ chainId,
+ address: String(t?.address ?? ''),
+ name: String(t?.name ?? ''),
+ symbol: String(t?.symbol ?? ''),
+ decimals: Number.isFinite(t?.decimals) ? Number(t.decimals) : 18,
+ logoURI: typeof t?.logoURI === 'string' ? t.logoURI : '',
+ }));📝 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.
| return data.tokens.map((token: TokenFromAPI) => ({ | |
| ...token, | |
| chainId, | |
| })); | |
| // Normalize mapped fields and provide safe defaults | |
| const { tokens } = data as { tokens: any[] }; | |
| return tokens.map((t: any): TokenFromAPI => ({ | |
| chainId, | |
| address: String(t?.address ?? ''), | |
| name: String(t?.name ?? ''), | |
| symbol: String(t?.symbol ?? ''), | |
| decimals: Number.isFinite(t?.decimals) ? Number(t.decimals) : 18, | |
| logoURI: typeof t?.logoURI === 'string' ? t.logoURI : '', | |
| })); |
🤖 Prompt for AI Agents
In libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts around
lines 30 to 33, the mapper blindly spreads upstream TokenFromAPI into our Token
contract; instead explicitly map each expected field (e.g., id, address, symbol,
name, decimals, logoURI, chainId) coercing types and applying safe defaults —
parse/Number.decimals fallback to 0, address normalized to lowercase and
empty-string fallback, symbol/name/logoURI default to empty string, chainId set
from the outer scope or 0 if missing — and return only the mapped properties to
ensure our contract and types are enforced.
| return TOKEN_MAP[chainId].filter( | ||
| (token) => | ||
| token.name.toLowerCase().includes(trimmedSearchParam) || | ||
| token.symbol.toLowerCase().includes(trimmedSearchParam) || | ||
| token.address.toLowerCase().includes(trimmedSearchParam) | ||
| ); |
There was a problem hiding this comment.
Guard against undefined chain entries to avoid runtime crash
If TOKEN_MAP[chainId] is missing, .filter will throw. Add a safe fallback.
Apply:
- return TOKEN_MAP[chainId].filter(
+ const list = TOKEN_MAP[chainId] ?? [];
+ return list.filter(
(token) =>
token.name.toLowerCase().includes(trimmedSearchParam) ||
token.symbol.toLowerCase().includes(trimmedSearchParam) ||
token.address.toLowerCase().includes(trimmedSearchParam)
);📝 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.
| return TOKEN_MAP[chainId].filter( | |
| (token) => | |
| token.name.toLowerCase().includes(trimmedSearchParam) || | |
| token.symbol.toLowerCase().includes(trimmedSearchParam) || | |
| token.address.toLowerCase().includes(trimmedSearchParam) | |
| ); | |
| const list = TOKEN_MAP[chainId] ?? []; | |
| return list.filter( | |
| (token) => | |
| token.name.toLowerCase().includes(trimmedSearchParam) || | |
| token.symbol.toLowerCase().includes(trimmedSearchParam) || | |
| token.address.toLowerCase().includes(trimmedSearchParam) | |
| ); |
🤖 Prompt for AI Agents
In libs/repositories/src/datasources/tokenSearch/tokenListSearch.ts around lines
11 to 16, the code calls TOKEN_MAP[chainId].filter(...) which will throw if
TOKEN_MAP[chainId] is undefined; update the return to safely handle missing
entries by defaulting to an empty array (e.g. use TOKEN_MAP[chainId] || [] or
optional chaining with ?? []) before calling .filter, so when the chainId key is
absent the function returns an empty array instead of crashing.
| const trimmedSearchParam = searchParam.trim().toLowerCase(); | ||
|
|
||
| return tokens.filter( | ||
| (token) => | ||
| token.name.toLowerCase().includes(trimmedSearchParam) || | ||
| token.symbol.toLowerCase().includes(trimmedSearchParam) || | ||
| token.address.toLowerCase().includes(trimmedSearchParam) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent empty queries from returning the entire token set.
This avoids accidental heavy responses and large JSON parses.
const trimmedSearchParam = searchParam.trim().toLowerCase();
- return tokens.filter(
+ if (!trimmedSearchParam) {
+ return [];
+ }
+
+ return tokens.filter(
(token) =>
token.name.toLowerCase().includes(trimmedSearchParam) ||
token.symbol.toLowerCase().includes(trimmedSearchParam) ||
token.address.toLowerCase().includes(trimmedSearchParam)
);📝 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 trimmedSearchParam = searchParam.trim().toLowerCase(); | |
| return tokens.filter( | |
| (token) => | |
| token.name.toLowerCase().includes(trimmedSearchParam) || | |
| token.symbol.toLowerCase().includes(trimmedSearchParam) || | |
| token.address.toLowerCase().includes(trimmedSearchParam) | |
| ); | |
| } | |
| const trimmedSearchParam = searchParam.trim().toLowerCase(); | |
| if (!trimmedSearchParam) { | |
| return []; | |
| } | |
| return tokens.filter( | |
| (token) => | |
| token.name.toLowerCase().includes(trimmedSearchParam) || | |
| token.symbol.toLowerCase().includes(trimmedSearchParam) || | |
| token.address.toLowerCase().includes(trimmedSearchParam) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts
around lines 58 to 66, the current filter uses searchParam.trim().toLowerCase()
but does not guard against empty or whitespace-only queries which causes the
entire token list to be returned; add a check after computing trimmedSearchParam
that if it's an empty string (or null/undefined) the function returns an empty
array (or a controlled empty result) immediately instead of running the filter,
and ensure searchParam is safely handled (e.g., coerce to string) before
trim/lowercase so empty queries cannot produce large responses.
| const ONE_HOUR = ms('1h'); | ||
| const ONE_DAY = ms('1d'); | ||
| const BUFFER_ERROR_TOLERANCE = 1.5; // 50% error tolerance | ||
| const BUFFER_ERROR_TOLERANCE = 1.8; // 80% error tolerance |
There was a problem hiding this comment.
Fix tolerance: 1.8 contradicts “80%” and neuters the gap assertion
With the current assertion shape abs(delta - interval) <= interval * BUFFER_ERROR_TOLERANCE, setting BUFFER_ERROR_TOLERANCE = 1.8 allows gaps from 0 up to 2.8× the target interval to pass, including duplicate timestamps. The comment says “80%,” which matches 0.8, not 1.8.
Set it to 0.8 to restore meaning and prevent zero/near-zero gaps from passing.
-const BUFFER_ERROR_TOLERANCE = 1.8; // 80% error tolerance
+const BUFFER_ERROR_TOLERANCE = 0.8; // 80% relative tolerance📝 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 BUFFER_ERROR_TOLERANCE = 1.8; // 80% error tolerance | |
| // libs/repositories/src/repos/UsdRepository/UsdRepositoryCoingecko.test.ts | |
| const BUFFER_ERROR_TOLERANCE = 0.8; // 80% relative tolerance |
🤖 Prompt for AI Agents
In libs/repositories/src/repos/UsdRepository/UsdRepositoryCoingecko.test.ts
around line 10, the BUFFER_ERROR_TOLERANCE constant is incorrectly set to 1.8
while the comment and intended logic describe 80% tolerance; this allows gaps
from 0 up to 2.8× the interval to pass. Change BUFFER_ERROR_TOLERANCE to 0.8 so
the assertion abs(delta - interval) <= interval * BUFFER_ERROR_TOLERANCE
enforces ±80% tolerance and prevents zero/near-zero gaps from being accepted.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
libs/services/src/factories.ts (1)
71-73: Remove sensitive/noisy logging;redisClient.authis not a safe/valid propertyLeaking auth info is a security risk and may not exist on ioredis instances (TS error). Drop the log.
export function getCacheRepository(): CacheRepository { if (redisClient) { - console.log('redisClient', redisClient.auth); return new CacheRepositoryRedis(redisClient); }
🧹 Nitpick comments (23)
apps/token-list-updater/tsconfig.app.json (1)
3-7: Enable composite for project referencesSince this project is referenced via tsconfig “references”, set composite: true on the referenced config to support tsc --build flows.
Apply:
"compilerOptions": { "outDir": "../../dist/out-tsc", "module": "commonjs", + "composite": true, "types": ["node"] },apps/token-list-updater/tsconfig.spec.json (1)
3-7: Addcomposite: trueto both tsconfig files
Include"composite": trueundercompilerOptionsin
- apps/token-list-updater/tsconfig.app.json
- apps/token-list-updater/tsconfig.spec.json
This flag is harmless for Jest and required for full
tsc --buildproject-reference support.apps/token-list-updater/Dockerfile (3)
9-11: Production install and runtime hints.
- Prefer
npm ciwith the generated lockfile; drop-f.- Set
NODE_ENV=production.- Optionally expose the port for clarity.
Apply:
ENV HOST=0.0.0.0 ENV PORT=3000 +ENV NODE_ENV=production @@ -RUN npm --prefix token-list-updater --omit=dev -f install +RUN npm --prefix token-list-updater ci --omit=dev + +EXPOSE 3000Also applies to: 20-23
17-23: Build artifact assumption: ensure dist exists before docker build.This Dockerfile expects
dist/apps/token-list-updaterto be present. If you usedocker compose build, it will fail unlessnx build token-list-updaterran first. Consider a multi-stage build, or document/enforce a pre-build step in CI.I can provide a minimal multi‑stage Dockerfile if you want to build inside the image.
7-7: Optional: pin base image by digest.Pin
node:lts-alpineto a digest to avoid surprise upgrades.docker-compose.yml (1)
160-172: Token-list-updater depends on prebuilt artifacts and service readiness.
- Build: The Dockerfile depends on a prebuilt
dist. Ensure CI runsnx build token-list-updaterbeforedocker compose buildor switch to a multi-stage Dockerfile.- Readiness: If the updater requires Redis at start, gate on health with
depends_onconditions (and a valid Redis healthcheck).Apply inside this service:
token-list-updater: @@ - depends_on: - - api - - redis + depends_on: + api: + condition: service_started + redis: + condition: service_healthyAnd outside this hunk (in the redis service), fix the healthcheck:
- healthcheck: - test: ['CMD', 'curl', '-f', 'http://localhost:6379'] + healthcheck: + test: ['CMD', 'redis-cli', 'ping'] interval: 30s timeout: 10s retries: 5apps/token-list-updater/project.json (1)
12-26: Optional: bundle to drop container-time npm install.If you flip
bundleto true, you can remove the Dockernpm installstep and ship a smaller, faster image.Apply here:
- "bundle": false, + "bundle": true,And in
apps/token-list-updater/Dockerfile, remove the install step:-# You can remove this install step if you build with `--bundle` option. -# The bundled output will include external dependencies. -RUN npm --prefix token-list-updater --omit=dev -f installAlso applies to: 28-38
apps/token-list-updater-e2e/tsconfig.spec.json (1)
4-4: Normalize path: remove duplicate slash in outDirDouble slash is harmless but noisy in diffs and tooling.
- "outDir": "../..//dist/out-tsc", + "outDir": "../../dist/out-tsc",apps/token-list-updater-e2e/src/token-list-updater/token-list-updater.spec.ts (1)
10-10: Align assertion with real CLI output“Hello World” is likely a placeholder and brittle. Prefer a stable log line or just assert successful execution.
Examples:
- expect(output).toMatch(/Hello World/); + expect(output).toMatch(/Token list updated/i);or
- expect(output).toMatch(/Hello World/); + expect(typeof output).toBe('string'); // ran without throwinglibs/services/src/factories.ts (2)
70-77: Avoid re-instantiating CacheRepository on every callMirror the singleton pattern used elsewhere to reuse the same cache repository.
// Singleton instances let postgresPool: Pool | undefined = undefined; let telegramBot: TelegramBot | undefined = undefined; +let sharedCacheRepository: CacheRepository | null = null; export function getCacheRepository(): CacheRepository { if (redisClient) { - return new CacheRepositoryRedis(redisClient); + if (!sharedCacheRepository) { + sharedCacheRepository = new CacheRepositoryRedis(redisClient); + } + return sharedCacheRepository; } return new CacheRepositoryMemory(); }
185-199: Clarify the error for missing Redis with actionable guidanceThe thrown message is correct but can point to envs to speed up ops/debug.
- throw new Error( - 'Redis client is required for TokenCacheRepository. Please configure Redis.' - ); + throw new Error( + 'Redis client is required for TokenCacheRepository. Configure Redis via REDIS_HOST/REDIS_PORT and optional REDIS_USER/REDIS_PASSWORD (or disable token-search features in this environment).' + );apps/token-list-updater-e2e/jest.config.ts (1)
4-4: Normalize paths: remove duplicate slashesMinor cleanup to avoid noisy diffs; functionally equivalent.
- preset: '../..//jest.preset.js', + preset: '../../jest.preset.js', ... - coverageDirectory: '../..//coverage/token-list-updater-e2e', + coverageDirectory: '../../coverage/token-list-updater-e2e',Also applies to: 16-16
apps/token-list-updater/src/updater/updater.ts (3)
7-10: Tighten type: use SupportedChainId instead of numberThis avoids passing unsupported chains at compile-time.
-import { initTokenList, TokenCacheRepository } from '@cowprotocol/repositories'; +import { initTokenList, TokenCacheRepository } from '@cowprotocol/repositories'; +import type { SupportedChainId } from '@cowprotocol/repositories'; ... export type TokenListUpdaterProps = { - chainId: number; + chainId: SupportedChainId; tokenCacheRepository: TokenCacheRepository; };
38-41: Unify logger call into a single messageCurrent
logger.info(msg, 'stopped')may be ignored or printed oddly depending on logger signature.- logger.info( - `TokenListUpdater for chain id: ${this.props.chainId}`, - 'stopped' - ); + logger.info(`TokenListUpdater for chain id: ${this.props.chainId} stopped`);
5-5: Consider making interval configurable and add jitterHard-coding 6h can hamper ops; env/config plus jitter reduces thundering herd when many instances restart together.
-const WAIT_TIME = 1000 * 60 * 60 * 6; // 6 hours +const BASE_WAIT_TIME = + Number(process.env.TOKEN_LIST_UPDATE_INTERVAL_MS) || 1000 * 60 * 60 * 6; // 6h +const jitter = Math.floor(BASE_WAIT_TIME * 0.1 * Math.random()); // ±10% +const WAIT_TIME = BASE_WAIT_TIME - Math.floor(BASE_WAIT_TIME * 0.05) + jitter;apps/token-list-updater/src/main.ts (5)
64-65: Use one‑shot signal handlers to prevent duplicate shutdowns.Prevents multiple invocations if handlers are re-registered (e.g., during future refactors or test harnesses).
- process.on('SIGTERM', shutdown); - process.on('SIGINT', shutdown); + process.once('SIGTERM', shutdown); + process.once('SIGINT', shutdown);
22-27: Align logging with actual updaters; derive both from the same source.Current log lists all supported chain IDs but only MAINNET is updated. Drive both the message and the updaters from a single chainIdsToUpdate array.
- const chainIds = ALL_SUPPORTED_CHAIN_IDS; - logger.info( - `[tokenlist-updater:main] Start tokenlist updater for networks: ${chainIds.join( - ', ' - )}` - ); + const chainIdsToUpdate: SupportedChainId[] = [SupportedChainId.MAINNET]; + logger.info( + `[tokenlist-updater:main] Start tokenlist updater for networks: ${chainIdsToUpdate.join(', ')}` + ); @@ - const updaters: Runnable[] = [ - new TokenListUpdater({ - chainId: SupportedChainId.MAINNET, - tokenCacheRepository, - }), - ]; + const updaters: Runnable[] = chainIdsToUpdate.map( + (chainId) => + new TokenListUpdater({ + chainId, + tokenCacheRepository, + }) + );Also applies to: 43-48
108-109: Fail fast on unhandled errors.Surface fatal issues to orchestrators and CI by exiting non‑zero.
-// Start the main loop -mainLoop().catch((error) => logger.error(error, 'Unhandled error in updater')); +// Start the main loop +mainLoop().catch((error) => { + logger.error(error, 'Unhandled error in updater'); + process.exit(1); +});
77-78: Comment consistency (producers → updaters).Minor rename for clarity.
- // Command all producers to stop + // Command all updaters to stop @@ - // Wait for all producers to actually stop + // Wait for all updaters to actually stopAlso applies to: 96-97
4-7: Remove unused import if adopting chainIdsToUpdate.If you switch the log/updater construction per the earlier suggestion,
ALL_SUPPORTED_CHAIN_IDSbecomes unused.-import { - ALL_SUPPORTED_CHAIN_IDS, - SupportedChainId, -} from '@cowprotocol/cow-sdk'; +import { SupportedChainId } from '@cowprotocol/cow-sdk';libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (2)
6-17: Clarify TTL semantics in the contract.The comment says “default: 24 hours” but that’s implementation-specific (Redis). Make this explicit to avoid cross-impl ambiguity.
/** * Initialize token list for a specific chain * @param chainId - The chain ID to initialize tokens for * @param tokens - Array of tokens to cache - * @param ttlSeconds - Time to live in seconds (default: 24 hours) + * @param ttlSeconds - Optional TTL in seconds. + * If omitted, the implementation's default is used (Redis impl: 24 hours).
26-35: Specify expected search behavior for consistency across implementations.Define case sensitivity, searchable fields, and ordering so Redis and any future impls behave the same.
/** - * Search tokens by parameter (name, symbol, or address) + * Search tokens by parameter (name, symbol, or address). + * Implementations should: + * - Perform case-insensitive matching. + * - Match by prefix or substring on name/symbol and by exact or checksum-insensitive match on address. + * - Return results ordered by relevance (e.g., exact symbol > prefix > substring). * @param chainId - The chain ID to search in * @param searchParam - Search parameter * @returns Array of matching tokens */libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
42-56: Input hygiene for search.Optional: trim and short-circuit empty queries to avoid expensive full scans in some backends.
export async function getTokenListBySearchParam( chainId: SupportedChainId, searchParam: string, cacheRepository?: TokenCacheRepository ): Promise<TokenFromAPI[]> { const repository = cacheRepository || tokenCacheRepository; if (!repository) { throw new Error( 'TokenCacheRepository not configured. Please set it using setTokenCacheRepository()' ); } - return repository.searchTokens(chainId, searchParam); + const q = searchParam?.trim(); + if (!q) return []; + return repository.searchTokens(chainId, q); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
apps/api/src/main.ts(2 hunks)apps/token-list-updater-e2e/.eslintrc.json(1 hunks)apps/token-list-updater-e2e/jest.config.ts(1 hunks)apps/token-list-updater-e2e/project.json(1 hunks)apps/token-list-updater-e2e/src/test-setup.ts(1 hunks)apps/token-list-updater-e2e/src/token-list-updater/token-list-updater.spec.ts(1 hunks)apps/token-list-updater-e2e/tsconfig.json(1 hunks)apps/token-list-updater-e2e/tsconfig.spec.json(1 hunks)apps/token-list-updater/.eslintrc.json(1 hunks)apps/token-list-updater/Dockerfile(1 hunks)apps/token-list-updater/jest.config.ts(1 hunks)apps/token-list-updater/project.json(1 hunks)apps/token-list-updater/src/main.ts(1 hunks)apps/token-list-updater/src/updater/updater.ts(1 hunks)apps/token-list-updater/tsconfig.app.json(1 hunks)apps/token-list-updater/tsconfig.json(1 hunks)apps/token-list-updater/tsconfig.spec.json(1 hunks)apps/token-list-updater/types.ts(1 hunks)docker-compose.yml(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.test.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts(1 hunks)libs/services/src/factories.ts(6 hunks)package.json(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/token-list-updater-e2e/.eslintrc.json
- apps/token-list-updater-e2e/tsconfig.json
- apps/token-list-updater-e2e/src/test-setup.ts
- apps/token-list-updater/.eslintrc.json
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts
- libs/repositories/src/datasources/tokenSearch/tokenList.test.ts
- apps/api/src/main.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-22T11:36:04.042Z
Learnt from: anxolin
PR: cowprotocol/bff#124
File: libs/services/src/factories.ts:124-126
Timestamp: 2025-04-22T11:36:04.042Z
Learning: Simulation repositories (SimulationRepositoryTenderly) don't require a caching layer like other repositories in the system, as the performance benefit isn't worth the implementation effort.
Applied to files:
libs/services/src/factories.ts
🧬 Code graph analysis (5)
apps/token-list-updater/src/main.ts (4)
libs/shared/src/logger.ts (1)
logger(3-3)libs/services/src/factories.ts (1)
getTokenCacheRepository(187-199)apps/token-list-updater/types.ts (1)
Runnable(4-14)apps/token-list-updater/src/updater/updater.ts (1)
TokenListUpdater(12-55)
libs/services/src/factories.ts (2)
libs/repositories/src/datasources/redis.ts (1)
redisClient(9-17)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)
libs/repositories/src/datasources/tokenSearch/tokenList.ts (5)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (1)
initTokenList(18-27)libs/repositories/src/datasources/coingecko.ts (1)
SUPPORTED_COINGECKO_PLATFORMS(10-23)libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (1)
getTokensByChainName(36-41)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
apps/token-list-updater/src/updater/updater.ts (3)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)apps/token-list-updater/types.ts (1)
Runnable(4-14)libs/shared/src/utils/doForever.ts (1)
doForever(4-41)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
🔇 Additional comments (6)
package.json (1)
17-17: Nx targettoken-list-updater:startverified
apps/token-list-updater/project.json defines astarttarget, so the script will succeed.apps/token-list-updater/tsconfig.json (1)
13-15: LGTM: esModuleInterop enabledMatches commonjs usage in app/spec configs and typical Node setup. No changes needed.
docker-compose.yml (1)
166-167: Verify.env.dockercompleteness
I wasn’t able to locate a.env.dockerfile or anyprocess.envusages inapps/token-list-updatervia automation—please manually ensure that the.env.dockeryou mount into Docker lists every variable your updater actually consumes (e.g. REDIS_HOST, REDIS_PORT, REDIS_USER, REDIS_PASSWORD, refresh interval, chains, etc.).apps/token-list-updater/jest.config.ts (1)
2-11: ts-jest and Jest major versions align. ts-jest v29.x (installed: ^29.1.0) requires and supports Jest 29.x (installed: ^29.5.0) (github.com)apps/token-list-updater/project.json (1)
76-79: Remove explicit executor;commandshorthand is supported
Thecommandfield inproject.jsonis the official shorthand for thenx:run-commandsexecutor and works out-of-the-box across Nx versions. (nx.dev, 19.nx.dev)Likely an incorrect or invalid review comment.
apps/token-list-updater-e2e/project.json (1)
1-22: Scaffold looks good.Implicit dep on app and Jest wiring are fine for now.
| import { execSync } from 'child_process'; | ||
| import { join } from 'path'; | ||
|
|
||
| describe('CLI tests', () => { | ||
| it('should print a message', () => { | ||
| const cliPath = join(process.cwd(), 'dist/apps/token-list-updater'); | ||
|
|
||
| const output = execSync(`node ${cliPath}`).toString(); | ||
|
|
||
| expect(output).toMatch(/Hello World/); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Point to the built entry file and use execFileSync with a timeout
The current test runs a directory path via a shell. Use the compiled main file and avoid shell interpolation; add a timeout to prevent hangs.
-import { execSync } from 'child_process';
+import { execFileSync } from 'child_process';
import { join } from 'path';
describe('CLI tests', () => {
it('should print a message', () => {
- const cliPath = join(process.cwd(), 'dist/apps/token-list-updater');
-
- const output = execSync(`node ${cliPath}`).toString();
+ const cliPath = join(process.cwd(), 'dist/apps/token-list-updater/main.js');
+ const output = execFileSync('node', [cliPath], {
+ encoding: 'utf8',
+ timeout: 15_000,
+ });
expect(output).toMatch(/Hello World/);
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { execSync } from 'child_process'; | |
| import { join } from 'path'; | |
| describe('CLI tests', () => { | |
| it('should print a message', () => { | |
| const cliPath = join(process.cwd(), 'dist/apps/token-list-updater'); | |
| const output = execSync(`node ${cliPath}`).toString(); | |
| expect(output).toMatch(/Hello World/); | |
| }); | |
| import { execFileSync } from 'child_process'; | |
| import { join } from 'path'; | |
| describe('CLI tests', () => { | |
| it('should print a message', () => { | |
| const cliPath = join(process.cwd(), 'dist/apps/token-list-updater/main.js'); | |
| const output = execFileSync('node', [cliPath], { | |
| encoding: 'utf8', | |
| timeout: 15_000, | |
| }); | |
| expect(output).toMatch(/Hello World/); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/token-list-updater-e2e/src/token-list-updater/token-list-updater.spec.ts
around lines 1–11, the test invokes a directory via execSync with shell
interpolation and no timeout; change it to point to the compiled entry file
(e.g. join(process.cwd(), 'dist/apps/token-list-updater', 'main.js')) and use
execFileSync to call the Node binary with the entry file as an argument (no
shell interpolation), passing a timeout option (e.g. 5000 ms) to avoid hangs.
| RUN addgroup --system token-list-updater && \ | ||
| adduser --system -G token-list-updater token-list-updater | ||
|
|
||
| COPY dist/apps/token-list-updater token-list-updater | ||
| RUN chown -R token-list-updater:token-list-updater . | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Run as non-root for container hardening.
You create the user and chown but never switch to it.
Apply:
RUN addgroup --system token-list-updater && \
adduser --system -G token-list-updater token-list-updater
@@
RUN chown -R token-list-updater:token-list-updater .
+
+USER token-list-updater:token-list-updater📝 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.
| RUN addgroup --system token-list-updater && \ | |
| adduser --system -G token-list-updater token-list-updater | |
| COPY dist/apps/token-list-updater token-list-updater | |
| RUN chown -R token-list-updater:token-list-updater . | |
| RUN addgroup --system token-list-updater && \ | |
| adduser --system -G token-list-updater token-list-updater | |
| COPY dist/apps/token-list-updater token-list-updater | |
| RUN chown -R token-list-updater:token-list-updater . | |
| USER token-list-updater:token-list-updater |
🤖 Prompt for AI Agents
In apps/token-list-updater/Dockerfile around lines 14 to 19, you create a system
user/group and chown the app files but never switch the container to run as that
non-root user; update the Dockerfile to set the working directory to the copied
app (ensure it's owned by token-list-updater) and add a USER token-list-updater
instruction after the chown so the container runs as the non-root
token-list-updater user.
| const timeoutInGracePeriod = new Promise((resolve) => | ||
| setTimeout(() => { | ||
| logger.info( | ||
| `Some of the updaters did not stop in time (${ | ||
| TIMEOUT_STOP_UPDATERS / 1000 | ||
| }s), forcing exit` | ||
| ); | ||
| resolve(true); | ||
| }, TIMEOUT_STOP_UPDATERS) | ||
| ); | ||
|
|
||
| await Promise.race([ | ||
| // Wait for all producers to actually stop | ||
| stopUpdatersPromise | ||
| .then(() => updatersPromise) | ||
| .then(() => logger.info('All updaters have been stopped')), | ||
| // Give some grace period (otherwise timeout) | ||
| timeoutInGracePeriod, | ||
| ]); | ||
|
|
||
| logger.info('Bye!'); | ||
| process.exit(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exit non‑zero on forced shutdown and surface the race outcome.
Right now a timeout path still exits with code 0, which hides shutdown failures. Return an explicit outcome from the raced promises and set the exit code accordingly.
- const timeoutInGracePeriod = new Promise((resolve) =>
+ const timeoutInGracePeriod = new Promise<'timeout'>((resolve) =>
setTimeout(() => {
logger.info(
`Some of the updaters did not stop in time (${
TIMEOUT_STOP_UPDATERS / 1000
}s), forcing exit`
);
- resolve(true);
+ resolve('timeout');
}, TIMEOUT_STOP_UPDATERS)
);
- await Promise.race([
- // Wait for all producers to actually stop
- stopUpdatersPromise
- .then(() => updatersPromise)
- .then(() => logger.info('All updaters have been stopped')),
- // Give some grace period (otherwise timeout)
- timeoutInGracePeriod,
- ]);
-
- logger.info('Bye!');
- process.exit(0);
+ const stoppedPromise = stopUpdatersPromise
+ .then(() => updatersPromise)
+ .then(() => {
+ logger.info('All updaters have been stopped');
+ return 'stopped' as const;
+ });
+
+ const outcome = await Promise.race([stoppedPromise, timeoutInGracePeriod]);
+
+ logger.info('Bye!');
+ process.exit(outcome === 'timeout' ? 1 : 0);Also applies to: 95-103
| async start(): Promise<void> { | ||
| await doForever({ | ||
| name: `TokenListUpdater for chain id: ${this.props.chainId}`, | ||
| callback: async (stop) => { | ||
| if (this.isStopping) { | ||
| stop(); | ||
| return; | ||
| } | ||
| logger.info(`Updating token list for chain id: ${this.props.chainId}`); | ||
| await this.updateTokenList(); | ||
| }, | ||
| waitTimeMilliseconds: WAIT_TIME, | ||
| logger, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Graceful stop may take up to WAIT_TIME (6h) — needs an external wake-up
stop() only flips a flag; doForever wakes early only when its internal stop() is invoked from the callback. If called during sleep, shutdown can be delayed by 6h.
Options:
- Extend
doForeverto return a controller withstop()that triggers its internalwakeUp(). - Or reduce
WAIT_TIME(e.g., to minutes) and add jitter, letting the callback decide when to run heavy work (e.g., skip until next scheduled tick). - Or wire a signal handler that invokes the provided
stop()inside the callback via an exposed controller.
I can draft a doForeverWithController helper if helpful.
| * Start the program, this method should not throw or finish. | ||
| */ | ||
| start(): Promise<void>; | ||
|
|
||
| /** | ||
| * Stop the program, this method should not throw or finish. | ||
| */ | ||
| stop(): Promise<void>; |
There was a problem hiding this comment.
Fix contradictory docs: stop() should finish
start(): “should not throw and should run until stopped.”
stop(): “should not throw and should resolve after shutdown.”
Apply:
/**
- * Start the program, this method should not throw or finish.
+ * Start the program. Should not throw and typically runs until stopped.
*/
start(): Promise<void>;
/**
- * Stop the program, this method should not throw or finish.
+ * Stop the program. Should not throw and should resolve after graceful shutdown.
*/
stop(): Promise<void>;📝 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.
| * Start the program, this method should not throw or finish. | |
| */ | |
| start(): Promise<void>; | |
| /** | |
| * Stop the program, this method should not throw or finish. | |
| */ | |
| stop(): Promise<void>; | |
| /** | |
| * Start the program. Should not throw and typically runs until stopped. | |
| */ | |
| start(): Promise<void>; | |
| /** | |
| * Stop the program. Should not throw and should resolve after graceful shutdown. | |
| */ | |
| stop(): Promise<void>; |
🤖 Prompt for AI Agents
In apps/token-list-updater/types.ts around lines 6 to 13, the JSDoc comments are
contradictory: update the comment for start() to state it should not throw and
should run until stopped (i.e. it returns a Promise that resolves only when the
program ends) and update the comment for stop() to state it should not throw and
should resolve after shutdown (i.e. it returns a Promise that resolves when
shutdown/cleanup completes); keep the method signatures unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (3)
9-14: Add a fetch timeout and abort handling (and set Accept header).Unbounded external calls can hang the updater. Abort after a sane limit and surface AbortError clearly.
+const FETCH_TIMEOUT_MS = 15_000; + logger.info(`Fetching tokens for ${chainName}`); - const response = await fetch(tokenSource); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); + let response: Response; + try { + response = await fetch(tokenSource, { + signal: controller.signal, + headers: { accept: 'application/json' }, + }); + } catch (e) { + if ((e as any)?.name === 'AbortError') { + throw new Error( + `Fetch aborted after ${FETCH_TIMEOUT_MS}ms: ${tokenSource}` + ); + } + throw e; + } finally { + clearTimeout(timeoutId); + }
21-27: Harden JSON parsing and shape validation.Catch parse errors and validate object shape before access.
- const data = await response.json(); - - if (!data.tokens || !Array.isArray(data.tokens)) { + let data: unknown; + try { + data = await response.json(); + } catch (e) { + throw new Error( + `Failed to parse token list JSON from ${tokenSource}: ${(e as Error).message}` + ); + } + if (!data || typeof data !== 'object' || !Array.isArray((data as any).tokens)) { throw new Error( `Invalid token list format from ${tokenSource}: missing or invalid tokens array` ); }
31-34: Map fields explicitly and coerce types to our contract.Avoid spreading upstream data blindly; normalize to TokenFromAPI.
- return data.tokens.map((token: TokenFromAPI) => ({ - ...token, - chainId, - })); + const { tokens } = data as { tokens: any[] }; + return tokens.map((t: any): TokenFromAPI => ({ + chainId, + address: String(t?.address ?? ''), + name: String(t?.name ?? ''), + symbol: String(t?.symbol ?? ''), + decimals: Number.isFinite(t?.decimals) ? Number(t.decimals) : 18, + logoURI: typeof t?.logoURI === 'string' ? t.logoURI : '', + }));apps/token-list-updater/src/main.ts (1)
77-99: Exit non‑zero on forced shutdown; surface outcome of the race.Timeout path currently exits 0, masking failures. Return the outcome and set exit code accordingly.
- const timeoutInGracePeriod = new Promise((resolve) => + const timeoutInGracePeriod = new Promise<'timeout'>((resolve) => setTimeout(() => { logger.info( `Some of the updaters did not stop in time (${ TIMEOUT_STOP_UPDATERS / 1000 }s), forcing exit` ); - resolve(true); + resolve('timeout'); }, TIMEOUT_STOP_UPDATERS) ); - await Promise.race([ - // Wait for all producers to actually stop - stopUpdatersPromise - .then(() => updatersPromise) - .then(() => logger.info('All updaters have been stopped')), - // Give some grace period (otherwise timeout) - timeoutInGracePeriod, - ]); - - logger.info('Bye!'); - process.exit(0); + const stoppedPromise = stopUpdatersPromise + .then(() => updatersPromise) + .then(() => { + logger.info('All updaters have been stopped'); + return 'stopped' as const; + }); + + const outcome = await Promise.race([stoppedPromise, timeoutInGracePeriod]); + + logger.info('Bye!'); + process.exit(outcome === 'timeout' ? 1 : 0);apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (1)
20-26: Spec now matches runtime (404 → 500). LGTM.
The response schema correctly documents 2XX and 500.
🧹 Nitpick comments (8)
libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (3)
9-10: Sanitize chainName in URL path.Guard against accidental path chars by encoding the segment.
- const tokenSource = `https://tokens.coingecko.com/${chainName}/all.json`; + const tokenSource = `https://tokens.coingecko.com/${encodeURIComponent( + chainName + )}/all.json`;
29-30: Tone down log level to reduce noise.This runs periodically; consider debug for per-run counts.
- logger.info(`Fetched ${data.tokens.length} tokens for ${chainName}`); + logger.debug(`Fetched ${data.tokens.length} tokens for ${chainName}`);
5-8: Consider conditional requests (ETag) to cut bandwidth.If CoinGecko supports ETag/If-None-Match, cache the ETag per chain and skip parsing on 304.
apps/token-list-updater/src/main.ts (2)
34-35: Avoid hard‑coding excluded chains.Prefer a config/env-driven allow/deny list to toggle networks without code changes.
49-59: Make shutdown handler idempotent and async‑aware.Minor: check shuttingDown before invoking, and wrap in void async to avoid unhandled rejections.
- const shutdown = () => { - gracefulShutdown(updaters, updatersPromise).catch((error) => { + const shutdown = () => { + if (shuttingDown) return; + void gracefulShutdown(updaters, updatersPromise).catch((error) => { logger.error(error, 'Error during shutdown'); process.exit(1); }); };apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (3)
34-36: Use request-scoped logger and lower verbosity.
Prefer request.log for correlation and debug level for high-volume searches.- fastify.log.info( + request.log.debug( `Token search for "${searchParam}" on chain ${chainId}: ${tokens.length} tokens found` ); ... - fastify.log.error('Error searching tokens:', error); + request.log.error({ err: error }, 'Error searching tokens');Also applies to: 40-41
20-26: Enrich OpenAPI metadata (summary/tags/operationId).
Helps generated clients and docs.schema: { + summary: 'Search tokens by name, symbol, or address', + tags: ['tokens', 'search'], + operationId: 'searchTokens', params: paramsSchema, response: { '2XX': successSchema, '500': errorSchema, }, },
38-39: Set caching headers for search responses.
Short TTL reduces load; tweak to your freshness needs.- reply.send(tokens); + reply.header('Cache-Control', 'public, max-age=300').send(tokens);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts(1 hunks)apps/token-list-updater/Dockerfile(1 hunks)apps/token-list-updater/src/main.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.ts(1 hunks)libs/services/src/factories.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/token-list-updater/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/services/src/factories.ts
- libs/repositories/src/datasources/tokenSearch/tokenList.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (1)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (6)
RouteSchema(79-79)SuccessSchema(80-80)ErrorSchema(81-81)paramsSchema(5-19)successSchema(21-64)errorSchema(66-77)
apps/token-list-updater/src/main.ts (4)
libs/shared/src/logger.ts (1)
logger(3-3)libs/services/src/factories.ts (1)
getTokenCacheRepository(186-198)apps/token-list-updater/types.ts (1)
Runnable(4-14)apps/token-list-updater/src/updater/updater.ts (1)
TokenListUpdater(12-55)
libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (2)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)libs/shared/src/logger.ts (1)
logger(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: docker (apps/notification-producer)
- GitHub Check: docker (apps/api)
- GitHub Check: docker (apps/twap)
- GitHub Check: docker (apps/telegram)
🔇 Additional comments (3)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (3)
12-17: Route scaffolding and typing look solid.
Good Fastify generics for Params/Reply and clear handler.
29-36: Verify AJVcoerceTypessetting and normalize inputs
No explicit AJVcoerceTypesconfiguration was found in the API; confirm it’s enabled in your Fastify/AJV setup—or coercechainIdto a number and trimsearchParamin apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts before passing them to getTokenListBySearchParam.
31-39: Ensure logoURI default in repository output
In getTokenListBySearchParam (libs/repositories/src/datasources/tokenSearch/tokenList.ts), map each token’s logoURI with a fallback (e.g.logoURI = token.logoURI ?? '') so the response schema’s required logoURI field is always set.
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (11)
libs/services/jest.config.ts (1)
9-11: Same ESM allowlist pattern — keep in sync or move to preset.Matches the repositories config. Prefer centralizing in
jest.preset.jsand ensureallowJs: trueas noted in the repositories comment.apps/notification-producer/jest.config.ts (1)
9-11: Consistent with other projects; consider centralizing.Identical pattern; recommend moving to
jest.preset.jsto avoid drift. See the repositories comment for details and a verification script.apps/token-list-updater/types.ts (1)
6-13: Fix contradictory JSDoc for start/stop lifecycleComments say “should not throw or finish” for both methods; suggest clarifying as below. (Echoing earlier feedback.)
/** - * Start the program, this method should not throw or finish. + * Start the program. Should not throw and typically runs until stopped. */ start(): Promise<void>; /** - * Stop the program, this method should not throw or finish. + * Stop the program. Should not throw and should resolve after graceful shutdown. */ stop(): Promise<void>;apps/token-list-updater/Dockerfile (1)
14-24: Run as non-root (add USER before CMD).You create the user and chown but never switch to it. Drop privileges for runtime.
RUN npm --prefix token-list-updater --omit=dev -f install +USER token-list-updater:token-list-updater + CMD [ "node", "token-list-updater/main.js" ]apps/api/src/app/inversify.config.ts (2)
58-58: Guard token-cache DI binding; don’t crash when Redis is absent.
This still hard-fails if Redis isn’t configured. Use a non-throwing factory and bind only when available; log a warning to align with graceful startup.- const tokenCacheRepository = getTokenCacheRepository(); + const tokenCacheRepository = tryGetTokenCacheRepository(); ... - apiContainer - .bind<TokenCacheRepository>(tokenCacheRepositorySymbol) - .toConstantValue(tokenCacheRepository); + if (tokenCacheRepository) { + apiContainer + .bind<TokenCacheRepository>(tokenCacheRepositorySymbol) + .toConstantValue(tokenCacheRepository); + } else { + logger.warn('Token cache repository not initialized; proceeding without Redis'); + }Also applies to: 92-95
7-7: Align imports with optional factory and set global for non-DI consumers.
Swap to tryGet factory; optionally set the global repository once to satisfy modules that use setTokenCacheRepository().-import { getTokenCacheRepository, +import { tryGetTokenCacheRepository, getCacheRepository, getErc20Repository, getPushNotificationsRepository, getPushSubscriptionsRepository, getSimulationRepository, getTokenHolderRepository, getUsdRepository } from '@cowprotocol/services'; @@ - tokenCacheRepositorySymbol, + tokenCacheRepositorySymbol, + setTokenCacheRepository,After creating/binding (inside the if), set the global once:
+ if (tokenCacheRepository) { + setTokenCacheRepository(tokenCacheRepository); + }Also applies to: 26-27
apps/token-list-updater-e2e/src/token-list-updater/token-list-updater.spec.ts (1)
4-12: Point to built entry and avoid shell interpolation; add timeout.
Use execFileSync against dist/apps/token-list-updater/main.js with a timeout.-import { execSync } from 'child_process'; +import { execFileSync } from 'child_process'; import { join } from 'path'; describe('CLI tests', () => { // TODO: implement properly this test it.skip('should print a message', () => { - const cliPath = join(process.cwd(), 'dist/apps/token-list-updater'); - - const output = execSync(`node ${cliPath}`).toString(); + const cliPath = join(process.cwd(), 'dist/apps/token-list-updater/main.js'); + const output = execFileSync('node', [cliPath], { + encoding: 'utf8', + timeout: 15_000, + }); expect(output).toMatch(/Hello World/); }); });libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (1)
9-35: Add fetch timeout, harden JSON parsing/shape, and normalize mapping.
External calls must have timeouts; JSON parse can throw; map/normalize upstream fields to our contract.Add near the top (file scope):
const FETCH_TIMEOUT_MS = 15_000;Apply:
- const tokenSource = `https://tokens.coingecko.com/${chainName}/all.json`; + const safeChain = encodeURIComponent(chainName); + const tokenSource = `https://tokens.coingecko.com/${safeChain}/all.json`; @@ - const response = await fetch(tokenSource); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); + let response: Response; + try { + response = await fetch(tokenSource, { + signal: controller.signal, + headers: { accept: 'application/json' }, + }); + } catch (err: any) { + if (err?.name === 'AbortError') { + throw new Error( + `Timeout fetching tokens from ${tokenSource} after ${FETCH_TIMEOUT_MS}ms` + ); + } + throw err; + } finally { + clearTimeout(timeoutId); + } @@ - const data = await response.json(); + let data: unknown; + try { + data = await response.json(); + } catch (e) { + throw new Error( + `Failed to parse token list JSON from ${tokenSource}: ${(e as Error).message}` + ); + } @@ - if (!data.tokens || !Array.isArray(data.tokens)) { + if (!data || typeof data !== 'object' || !Array.isArray((data as any).tokens)) { throw new Error( `Invalid token list format from ${tokenSource}: missing or invalid tokens array` ); } @@ - return data.tokens.map((token: TokenFromAPI) => ({ - ...token, - chainId, - })); + const { tokens } = data as { tokens: any[] }; + return tokens.map((t: any): TokenFromAPI => ({ + chainId, + address: String(t?.address ?? '').toLowerCase(), + name: String(t?.name ?? ''), + symbol: String(t?.symbol ?? ''), + decimals: Number.isFinite(t?.decimals) ? Number(t.decimals) : 18, + logoURI: typeof t?.logoURI === 'string' ? t.logoURI : '', + }));apps/token-list-updater/src/main.ts (1)
78-100: Exit non-zero on forced shutdown; return and act on race outcomeTimeout path currently exits 0, hiding shutdown failures. Race should return an outcome and set exit code accordingly.
- const timeoutInGracePeriod = new Promise((resolve) => + const timeoutInGracePeriod = new Promise<'timeout'>((resolve) => setTimeout(() => { logger.info( `Some of the updaters did not stop in time (${ TIMEOUT_STOP_UPDATERS / 1000 }s), forcing exit` ); - resolve(true); + resolve('timeout'); }, TIMEOUT_STOP_UPDATERS) ); - await Promise.race([ - // Wait for all producers to actually stop - stopUpdatersPromise - .then(() => updatersPromise) - .then(() => logger.info('All updaters have been stopped')), - // Give some grace period (otherwise timeout) - timeoutInGracePeriod, - ]); - - logger.info('Bye!'); - process.exit(0); + const stoppedPromise = stopUpdatersPromise + .then(() => updatersPromise) + .then(() => { + logger.info('All updaters have been stopped'); + return 'stopped' as const; + }); + + const outcome = await Promise.race([stoppedPromise, timeoutInGracePeriod]); + + logger.info('Bye!'); + process.exit(outcome === 'timeout' ? 1 : 0);apps/token-list-updater/src/updater/updater.ts (1)
24-41: Graceful stop can take up to 6h; expose doForever controller to wake early.
stop()only flips a flag; if called during sleep, shutdown waits until WAIT_TIME elapses. ExposedoForever’s internalstop()/wakeUp()externally and invoke it insidestop()here for immediate wake-and-exit.Example changes in this file (paired with a doForever change shown below):
export class TokenListUpdater implements Runnable { isStopping = false; + private controller?: { stop: () => void }; constructor(private props: TokenListUpdaterProps) {} async start(): Promise<void> { - await doForever({ + this.controller = await doForever({ name: `TokenListUpdater for chain id: ${this.props.chainId}`, callback: async (stop) => { if (this.isStopping) { stop(); return; } logger.info(`Updating token list for chain id: ${this.props.chainId}`); - await (async () => { - await sleep(this.props.delayInMilliseconds || 0); - await this.updateTokenList(); - })(); + await (async () => { + // delay handled below on first run + await this.updateTokenList(); + })(); }, waitTimeMilliseconds: WAIT_TIME, logger, });And update
stop()to wake:async stop(): Promise<void> { this.isStopping = true; + this.controller?.stop(); }Changes needed outside this file (libs/shared/src/utils/doForever.ts):
-export async function doForever(params: { ... }) { +export async function doForever(params: { ... }): Promise<{ stop: () => void }> { ... - while (running) { + while (running) { const stop = () => { logger.info(`[${name}] Stopping...`); wakeUp(); running = false; }; ... } logger.info(`[${name}] Stopped`); + return { stop: () => wakeUp() }; }libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (1)
79-86: Avoid returning the entire set for empty queries; optionally cap results.const trimmedSearchParam = searchParam.trim().toLowerCase(); - - return tokens.filter( + if (!trimmedSearchParam) { + return []; + } + const results = tokens.filter( (token) => token.name.toLowerCase().includes(trimmedSearchParam) || token.symbol.toLowerCase().includes(trimmedSearchParam) || token.address.toLowerCase().includes(trimmedSearchParam) ); + // Optional cap to prevent large payloads + return results.slice(0, 50);
🧹 Nitpick comments (22)
apps/token-list-updater-e2e/src/test-setup.ts (1)
1-1: Make this a module to avoid accidental global augmentationsA bare comment is fine, but exporting an empty object hardens the file against future TS global merges.
-// +export {}package.json (2)
17-17: Script naming consistencyConsider aligning with existing "start:*" convention for discoverability (optional).
- "token-list-updater": "nx run token-list-updater:start", + "start:token-list-updater": "nx run token-list-updater:start",
114-115: Enforce the pinned package managerYou’ve pinned Yarn via packageManager. Ensure contributors use Corepack so the exact Yarn version is respected in CI/dev.
libs/repositories/src/datasources/tokenSearch/types.ts (1)
3-10: Validate source shape or make logoURI optionalDo CoinGecko lists always include logoURI? If not, make it optional or add normalization to guarantee it.
export type TokenFromAPI = { chainId: SupportedChainId; address: string; name: string; symbol: string; decimals: number; - logoURI: string; + logoURI?: string; };apps/token-list-updater/tsconfig.json (1)
13-15: Confirm esModuleInterop alignment with workspaceEnsure this override matches the repo-wide setting to avoid mixed module interop behavior between apps/libs.
apps/token-list-updater/jest.config.ts (1)
6-8: Optional: speed up ts-jest with isolatedModulesCan improve test startup time; safe when no TS type-checking needed at runtime.
transform: { - '^.+\\.[tj]s$': ['ts-jest', { tsconfig: '<rootDir>/tsconfig.spec.json' }], + '^.+\\.[tj]s$': ['ts-jest', { tsconfig: '<rootDir>/tsconfig.spec.json', isolatedModules: true }], },apps/token-list-updater/Dockerfile (2)
7-11: Pin the base image and set NODE_ENV=production.Improves reproducibility and defaults to prod mode.
-FROM docker.io/node:lts-alpine +FROM docker.io/node:22-alpine ENV HOST=0.0.0.0 ENV PORT=3000 +ENV NODE_ENV=production
9-11: Optionally EXPOSE the port for clarity.Not required, but documents intent and eases local runs.
ENV HOST=0.0.0.0 ENV PORT=3000 +EXPOSE 3000docker-compose.yml (1)
160-172: Tighten dependencies and (optionally) expose the updater if needed.
- If the updater does not call the API, remove that dependency to reduce coupling.
- Expose 3000 only if you intend to access it from host; otherwise omit.
- Confirm required REDIS_* env vars are present via env_file for this service.
token-list-updater: container_name: token-list-updater @@ networks: - bff-network depends_on: - - api - redis + # Optional: expose if you need host access to the updater HTTP server + # ports: + # - '3000:3000'apps/token-list-updater-e2e/tsconfig.spec.json (1)
4-4: Normalize path (double slash).Purely cosmetic.
- "outDir": "../..//dist/out-tsc", + "outDir": "../../dist/out-tsc",apps/token-list-updater/.eslintrc.json (1)
4-17: Remove redundant overrides.The first override already covers both TS and JS; the next two are no-ops.
"overrides": [ { "files": ["*.ts", "*.tsx", "*.js", "*.jsx"], "rules": {} }, - { - "files": ["*.ts", "*.tsx"], - "rules": {} - }, - { - "files": ["*.js", "*.jsx"], - "rules": {} - } ]libs/repositories/src/datasources/tokenSearch/index.ts (1)
1-1: Optionally re-export types for consumers.If you want external callers to use the token search types without deep imports, consider re-exporting them here.
Happy to add this if you confirm which types should be public.
libs/services/src/factories.ts (1)
190-204: Expose a non-throwing factory to enable optional DI bindings.
Current getTokenCacheRepository() throws without Redis, forcing all callers to try/catch. Provide tryGetTokenCacheRepository() and use undefined instead of null for the singleton.-let tokenCacheRepository: TokenCacheRepository | null = null; +let tokenCacheRepository: TokenCacheRepository | undefined; export function getTokenCacheRepository(): TokenCacheRepository { if (!redisClient) { throw new Error( 'Redis client is required for TokenCacheRepository. Please configure Redis.' ); } if (!tokenCacheRepository) { tokenCacheRepository = new TokenCacheRepositoryRedis(redisClient); } return tokenCacheRepository; } + +// Non-throwing variant for optional consumers (e.g., DI wiring / local dev) +export function tryGetTokenCacheRepository(): + | TokenCacheRepository + | undefined { + if (!redisClient) return undefined; + if (!tokenCacheRepository) { + tokenCacheRepository = new TokenCacheRepositoryRedis(redisClient); + } + return tokenCacheRepository; +}apps/token-list-updater-e2e/jest.config.ts (1)
4-4: Tiny path nit: remove duplicate slashes.
Harmless but noisy in diffs; normalize paths.- preset: '../..//jest.preset.js', + preset: '../../jest.preset.js', @@ - coverageDirectory: '../..//coverage/token-list-updater-e2e', + coverageDirectory: '../../coverage/token-list-updater-e2e',Also applies to: 16-16
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (2)
20-26: Spec mismatch: add 4XX error schema for validation failuresFastify will return 400 on params validation errors (e.g., minLength < 3). Document 4XX in the response schema so API consumers and generated clients aren’t surprised.
schema: { params: paramsSchema, response: { '2XX': successSchema, + '4XX': errorSchema, '500': errorSchema, }, },
31-36: Normalize input and reduce log noiseTrim the search term and log at debug to avoid noisy info logs on hot endpoints.
- const tokens = await getTokenListBySearchParam(chainId, searchParam); - - fastify.log.info( - `Token search for "${searchParam}" on chain ${chainId}: ${tokens.length} tokens found` - ); + const normalized = searchParam.trim(); + const tokens = await getTokenListBySearchParam(chainId, normalized); + + fastify.log.debug( + `Token search for "${normalized}" on chain ${chainId}: ${tokens.length} tokens found` + );apps/token-list-updater/src/main.ts (1)
58-59: Use process.once for signal handlersPrevents duplicate handlers if mainLoop is ever re-entered.
- process.on('SIGTERM', shutdown); - process.on('SIGINT', shutdown); + process.once('SIGTERM', shutdown); + process.once('SIGINT', shutdown);apps/token-list-updater/src/updater/updater.ts (2)
33-36: Apply startup delay only once; clamp negatives.As written, delay runs every cycle, not just on startup. If the goal is staggered boot, apply it once. Also guard against negative values.
- await (async () => { - await sleep(this.props.delayInMilliseconds || 0); - await this.updateTokenList(); - })(); + await (async () => { + const delay = Math.max(0, this.props.delayInMilliseconds ?? 0); + if (delay > 0) { + await sleep(delay); + // Zero out after first application + this.props = { ...this.props, delayInMilliseconds: 0 }; + } + await this.updateTokenList(); + })();
42-45: Unify log message into a single string.Many loggers treat the second arg as metadata; here you likely want a single message.
- logger.info( - `TokenListUpdater for chain id: ${this.props.chainId}`, - 'stopped' - ); + logger.info(`TokenListUpdater for chain id: ${this.props.chainId} stopped`);libs/repositories/src/datasources/tokenSearch/tokenList.test.ts (1)
17-19: Restore global.fetch after the suite to prevent cross-test leakage.-const mockFetch = jest.fn(); -global.fetch = mockFetch; +const mockFetch = jest.fn(); +const originalFetch = global.fetch; +global.fetch = mockFetch; @@ beforeEach(() => { jest.clearAllMocks(); mockFetch.mockClear(); @@ }); + + afterAll(() => { + global.fetch = originalFetch as any; + });Also applies to: 42-49
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (2)
11-17: Reject whitespace-only queries at schema level.Add a pattern requiring at least one non-space to avoid heavy empty searches slipping through as " ".
searchParam: { title: 'Search Parameter', description: 'Token search parameter (name, symbol, or address)', type: 'string', minLength: 3, maxLength: 100, + pattern: '.*\\S.*' },
40-61: Tighten success item validation (URI + non-empty names).name: { title: 'Name', description: 'Full name of the token.', - type: 'string', + type: 'string', + minLength: 1 }, symbol: { title: 'Symbol', description: 'Token symbol/ticker.', - type: 'string', + type: 'string', + minLength: 1 }, decimals: { @@ logoURI: { title: 'Logo URI', description: 'URI to the token logo.', - type: 'string', + type: 'string', + format: 'uri' },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
apps/api/src/app/inversify.config.ts(4 hunks)apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts(1 hunks)apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts(1 hunks)apps/api/src/main.ts(2 hunks)apps/notification-producer/jest.config.ts(1 hunks)apps/token-list-updater-e2e/.eslintrc.json(1 hunks)apps/token-list-updater-e2e/jest.config.ts(1 hunks)apps/token-list-updater-e2e/project.json(1 hunks)apps/token-list-updater-e2e/src/test-setup.ts(1 hunks)apps/token-list-updater-e2e/src/token-list-updater/token-list-updater.spec.ts(1 hunks)apps/token-list-updater-e2e/tsconfig.json(1 hunks)apps/token-list-updater-e2e/tsconfig.spec.json(1 hunks)apps/token-list-updater/.eslintrc.json(1 hunks)apps/token-list-updater/Dockerfile(1 hunks)apps/token-list-updater/jest.config.ts(1 hunks)apps/token-list-updater/project.json(1 hunks)apps/token-list-updater/src/main.ts(1 hunks)apps/token-list-updater/src/updater/updater.ts(1 hunks)apps/token-list-updater/tsconfig.app.json(1 hunks)apps/token-list-updater/tsconfig.json(1 hunks)apps/token-list-updater/tsconfig.spec.json(1 hunks)apps/token-list-updater/types.ts(1 hunks)docker-compose.yml(1 hunks)libs/repositories/jest.config.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/index.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.test.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/tokenList.ts(1 hunks)libs/repositories/src/datasources/tokenSearch/types.ts(1 hunks)libs/repositories/src/index.ts(2 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.test.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts(1 hunks)libs/repositories/src/repos/TokenCacheRepository/index.ts(1 hunks)libs/services/jest.config.ts(1 hunks)libs/services/src/factories.ts(5 hunks)package.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-22T11:36:04.042Z
Learnt from: anxolin
PR: cowprotocol/bff#124
File: libs/services/src/factories.ts:124-126
Timestamp: 2025-04-22T11:36:04.042Z
Learning: Simulation repositories (SimulationRepositoryTenderly) don't require a caching layer like other repositories in the system, as the performance benefit isn't worth the implementation effort.
Applied to files:
libs/services/src/factories.ts
🧬 Code graph analysis (13)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
libs/repositories/src/datasources/tokenSearch/tokenList.test.ts (4)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)libs/repositories/src/datasources/tokenSearch/tokenList.ts (3)
setTokenCacheRepository(10-14)initTokenList(16-39)getTokenListBySearchParam(41-55)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (1)
initTokenList(18-40)
libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (2)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)libs/shared/src/logger.ts (1)
logger(3-3)
apps/token-list-updater/src/main.ts (4)
libs/shared/src/logger.ts (1)
logger(3-3)libs/services/src/factories.ts (1)
getTokenCacheRepository(192-204)apps/token-list-updater/types.ts (1)
Runnable(4-14)apps/token-list-updater/src/updater/updater.ts (1)
TokenListUpdater(13-59)
apps/token-list-updater/src/updater/updater.ts (5)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)apps/token-list-updater/types.ts (1)
Runnable(4-14)libs/shared/src/utils/doForever.ts (1)
doForever(4-41)libs/shared/src/logger.ts (1)
logger(3-3)libs/shared/src/utils/misc.ts (1)
sleep(60-62)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (2)
apps/api/src/app/schemas.ts (1)
SupportedChainIdSchema(3-8)libs/shared/src/const.ts (1)
AllChainIds(57-57)
libs/repositories/src/datasources/tokenSearch/tokenList.ts (6)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (1)
initTokenList(18-40)libs/repositories/src/datasources/coingecko.ts (1)
SUPPORTED_COINGECKO_PLATFORMS(10-23)libs/shared/src/logger.ts (1)
logger(3-3)libs/repositories/src/datasources/tokenSearch/getTokensByChain.ts (1)
getTokensByChainName(37-42)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.test.ts (1)
libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
apps/api/src/main.ts (2)
libs/services/src/factories.ts (1)
getTokenCacheRepository(192-204)libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
setTokenCacheRepository(10-14)
apps/api/src/app/inversify.config.ts (2)
libs/services/src/factories.ts (1)
getTokenCacheRepository(192-204)libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (2)
TokenCacheRepository(6-42)tokenCacheRepositorySymbol(4-4)
libs/services/src/factories.ts (2)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)libs/repositories/src/datasources/redis.ts (1)
redisClient(9-17)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (2)
apps/api/src/app/routes/__chainId/tokens/search/__searchParam/schemas.ts (6)
RouteSchema(79-79)SuccessSchema(80-80)ErrorSchema(81-81)paramsSchema(5-19)successSchema(21-64)errorSchema(66-77)libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
getTokenListBySearchParam(41-55)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts (3)
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
TokenCacheRepository(6-42)libs/repositories/src/datasources/redis.ts (1)
redisClient(9-17)libs/repositories/src/datasources/tokenSearch/types.ts (1)
TokenFromAPI(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: docker (apps/api)
- GitHub Check: docker (apps/twap)
- GitHub Check: docker (apps/telegram)
- GitHub Check: docker (apps/notification-producer)
🔇 Additional comments (18)
libs/repositories/src/repos/TokenCacheRepository/index.ts (1)
1-2: Barrel looks goodClean re-exports; simplifies imports for consumers.
apps/token-list-updater/tsconfig.spec.json (1)
1-14: LGTMJest TS config aligns with ts-jest and CommonJS tests.
apps/token-list-updater/Dockerfile (1)
24-24: Entrypoint looks correct now.Running the built main file is the right call.
apps/token-list-updater-e2e/.eslintrc.json (1)
1-10: LGTM.Matches the repo’s ESLint structure for e2e.
apps/token-list-updater-e2e/tsconfig.json (1)
1-13: LGTM.Project refs and interop settings look fine.
libs/repositories/src/index.ts (2)
7-7: LGTM: public re-export for tokenSearch.Makes the data source accessible from the barrel.
70-71: LGTM: TokenCacheRepository exports.Barrel now exposes the interface and Redis impl.
apps/token-list-updater/tsconfig.app.json (1)
1-10: LGTM for app tsconfig.
Config looks standard for a Node/Nx app; no issues spotted.libs/services/src/factories.ts (2)
111-112: LGTM: CoW USD path ordering.
Including Cow as a fallback option alongside Coingecko is fine.
143-145: LGTM: TokenHolder repository fallback ordering.
Order looks intentional; no functional concerns.libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepository.ts (1)
6-41: Interface looks solid and minimal.
Clear contract with init/get/search/clear; good separation for Redis-backed impl.apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (1)
29-29: Verify AJVcoerceTypes: trueis enabled
Confirm in apps/api/src/app/routes/__chainId/tokens/search/__searchParam/index.ts (line 29) that your Fastify/AJV setup includescoerceTypes: true; otherwiserequest.params.chainIdremains a string and may break calls expecting a number.apps/token-list-updater-e2e/project.json (1)
1-22: LGTM — sane Nx e2e target and lint wiringConfig looks good and aligns with Nx defaults.
apps/token-list-updater/project.json (1)
29-37: Verify “generateLockfile” support in @nx/esbuild:esbuild.This option may not exist for the esbuild executor. If unsupported in your Nx version, remove it to avoid schema validation errors.
If unsupported, apply:
"production": { - "generateLockfile": true, "esbuildOptions": { "sourcemap": false, "outExtension": { ".js": ".js" } } }libs/repositories/src/datasources/tokenSearch/tokenList.test.ts (1)
297-329: Inconsistent contract: tests assume logoURI is optional but types and API schema require it.TokenFromAPI and the API success schema mark logoURI as required. Decide: either make logoURI optional everywhere or adjust this test to include it.
Options:
- Make logoURI optional across types/schemas, or
- Update the “optional fields missing” test to include logoURI.
libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.test.ts (2)
229-241: Align test with error handling via shared logger instead of console.log.After wrapping hgetall with try/catch and logging via logger.error, assert on logger rather than console.
-import { TokenFromAPI } from '../../datasources/tokenSearch/types'; +import { TokenFromAPI } from '../../datasources/tokenSearch/types'; +import { logger } from '@cowprotocol/shared'; @@ -const consoleSpy = { - log: jest.spyOn(console, 'log').mockImplementation(), - warn: jest.spyOn(console, 'warn').mockImplementation(), -}; +const consoleSpy = { + warn: jest.spyOn(console, 'warn').mockImplementation(), +}; +const loggerSpy = { + error: jest.spyOn(logger, 'error').mockImplementation(), + warn: jest.spyOn(logger, 'warn').mockImplementation(), +}; @@ beforeEach(() => { repository = new TokenCacheRepositoryRedis(mockRedisClient); jest.clearAllMocks(); - consoleSpy.log.mockClear(); consoleSpy.warn.mockClear(); + loggerSpy.error.mockClear(); + loggerSpy.warn.mockClear(); }); @@ - afterAll(() => { - consoleSpy.log.mockRestore(); - consoleSpy.warn.mockRestore(); - }); + afterAll(() => { + consoleSpy.warn.mockRestore(); + loggerSpy.error.mockRestore(); + loggerSpy.warn.mockRestore(); + }); @@ const result = await repository.getTokenList(chainId); @@ - expect(consoleSpy.log).toHaveBeenCalledWith( - 'redis getTokenList 5 ==>', - error - ); + expect(loggerSpy.error).toHaveBeenCalled();
319-331: If empty queries should not return all tokens, update expectations accordingly.Should you adopt the empty-query guard, change these two tests to expect an empty array.
- it('should handle empty search parameter', async () => { + it('should handle empty search parameter (returns empty)', async () => { const result = await repository.searchTokens(chainId, ''); - - expect(result).toHaveLength(3); - expect(result).toEqual(expect.arrayContaining(mockTokens)); + expect(result).toHaveLength(0); }); @@ - it('should handle whitespace-only search parameter', async () => { + it('should handle whitespace-only search parameter (returns empty)', async () => { const result = await repository.searchTokens(chainId, ' '); - - expect(result).toHaveLength(3); - expect(result).toEqual(expect.arrayContaining(mockTokens)); + expect(result).toHaveLength(0); });libs/repositories/src/datasources/tokenSearch/tokenList.ts (1)
16-39: Solid DI and logging; fetch-before-cache avoids downtime.
| const tokenCacheRepository = getTokenCacheRepository(); | ||
| setTokenCacheRepository(tokenCacheRepository); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unconditional Redis dependency here can crash API startup; remove wiring from main or guard it.
If REDIS_* isn’t set, getTokenCacheRepository() throws and the process exits before Fastify starts. Prefer sourcing the instance via DI only (recommended), or guard creation.
Recommended (remove from main; let inversify.config.ts own lifecycle):
-import { getTokenCacheRepository } from '@cowprotocol/services';
-import { setTokenCacheRepository } from '@cowprotocol/repositories';
...
-const tokenCacheRepository = getTokenCacheRepository();
-setTokenCacheRepository(tokenCacheRepository);If you must keep it in main, at least guard:
+import { getTokenCacheRepository } from '@cowprotocol/services';
+import { setTokenCacheRepository } from '@cowprotocol/repositories';
...
-const tokenCacheRepository = getTokenCacheRepository();
-setTokenCacheRepository(tokenCacheRepository);
+try {
+ const tokenCacheRepository = getTokenCacheRepository();
+ setTokenCacheRepository(tokenCacheRepository);
+ server.log.info('Token cache repository initialized');
+} catch (err) {
+ server.log.warn({ err }, 'Token cache repository not initialized; proceeding without Redis');
+}Also applies to: 4-5
🤖 Prompt for AI Agents
In apps/api/src/main.ts around lines 15 to 17, the call to
getTokenCacheRepository() unconditionally requires Redis and can throw if
REDIS_* env vars are missing; remove this wiring from main (let
inversify.config.ts/inversion of control own the lifecycle and inject the
repository where needed) or, if you must keep it here, guard creation by
checking the required REDIS_* env vars and only call
setTokenCacheRepository(...) when they are present (or wrap
getTokenCacheRepository() in a try/catch and skip wiring on failure), ensuring
Fastify startup is not blocked by a missing Redis configuration.
| "docker-build": { | ||
| "dependsOn": ["build"], | ||
| "command": "docker build -f apps/token-list-updater/Dockerfile . -t token-list-updater" | ||
| } |
There was a problem hiding this comment.
Invalid Nx target: “docker-build” lacks an executor; won’t run.
Nx targets require an executor (e.g., nx:run-commands). Add it and move the command under options.
- "docker-build": {
- "dependsOn": ["build"],
- "command": "docker build -f apps/token-list-updater/Dockerfile . -t token-list-updater"
- }
+ "docker-build": {
+ "dependsOn": ["build"],
+ "executor": "nx:run-commands",
+ "options": {
+ "command": "docker build -f apps/token-list-updater/Dockerfile . -t token-list-updater"
+ }
+ }📝 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.
| "docker-build": { | |
| "dependsOn": ["build"], | |
| "command": "docker build -f apps/token-list-updater/Dockerfile . -t token-list-updater" | |
| } | |
| "docker-build": { | |
| "dependsOn": ["build"], | |
| "executor": "nx:run-commands", | |
| "options": { | |
| "command": "docker build -f apps/token-list-updater/Dockerfile . -t token-list-updater" | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/token-list-updater/project.json around lines 76-79, the "docker-build"
target is missing an executor so Nx will not run it; change the target to
include an "executor": "nx:run-commands" (or another appropriate executor) and
move the "command" into an "options" object (preserving "dependsOn" at the
target level), e.g. add "executor": "nx:run-commands" and "options": {
"command": "docker build -f apps/token-list-updater/Dockerfile . -t
token-list-updater" } so Nx can execute the target.
| const tokenCacheRepository = getTokenCacheRepository(); | ||
|
|
||
| const updaters: Runnable[] = [ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail fast if TokenCacheRepository init fails (e.g., Redis not configured)
Currently this will throw later and the process may keep running. Exit with code 1 immediately.
- const tokenCacheRepository = getTokenCacheRepository();
+ let tokenCacheRepository: ReturnType<typeof getTokenCacheRepository>;
+ try {
+ tokenCacheRepository = getTokenCacheRepository();
+ } catch (err) {
+ logger.error(err, 'TokenCacheRepository init failed (Redis not configured)');
+ process.exit(1);
+ }📝 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 tokenCacheRepository = getTokenCacheRepository(); | |
| const updaters: Runnable[] = [ | |
| let tokenCacheRepository: ReturnType<typeof getTokenCacheRepository>; | |
| try { | |
| tokenCacheRepository = getTokenCacheRepository(); | |
| } catch (err) { | |
| logger.error(err, 'TokenCacheRepository init failed (Redis not configured)'); | |
| process.exit(1); | |
| } | |
| const updaters: Runnable[] = [ |
🤖 Prompt for AI Agents
In apps/token-list-updater/src/main.ts around lines 29 to 31, ensure the process
fails fast when TokenCacheRepository initialization fails: wrap the call to
getTokenCacheRepository() in a try/catch (or check for a falsy return) and if it
throws or returns null/undefined, log the error (prefer the existing process
logger if available, otherwise console.error) and call process.exit(1)
immediately so the process does not continue running with an invalid cache
repository.
| // Start the main loop | ||
| mainLoop().catch((error) => logger.error(error, 'Unhandled error in updater')); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exit non-zero on unhandled startup errors
Surface fatal startup/runtime errors to the orchestrator.
-mainLoop().catch((error) => logger.error(error, 'Unhandled error in updater'));
+mainLoop().catch((error) => {
+ logger.error(error, 'Unhandled error in updater');
+ process.exit(1);
+});📝 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.
| // Start the main loop | |
| mainLoop().catch((error) => logger.error(error, 'Unhandled error in updater')); | |
| // Start the main loop | |
| mainLoop().catch((error) => { | |
| logger.error(error, 'Unhandled error in updater'); | |
| process.exit(1); | |
| }); |
🤖 Prompt for AI Agents
In apps/token-list-updater/src/main.ts around lines 102-103, the main loop's
unhandled rejection handler only logs the error but does not exit with a
non-zero code; update the catch to log the full error (as currently done) and
then terminate the process with a non-zero exit (e.g., call process.exit(1)
immediately after logging, or schedule it via setImmediate/process.nextTick) so
the orchestrator sees the failure.
| transformIgnorePatterns: [ | ||
| 'node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)/)', | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Centralize the ESM allowlist and verify ts-jest can transpile JS.
- Optional DRY: move this identical
transformIgnorePatternstojest.preset.jsso all projects stay in sync. - Ensure
ts-jestcan actually transform the whitelisted JS from node_modules; confirmallowJs: truein eachtsconfig.spec.json. If any deps ship.mjs, you may need an additional transformer (e.g.,babel-jest) or ESM settings.
Apply if centralizing in jest.preset.js:
- transformIgnorePatterns: [
- 'node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)/)',
- ],Add to jest.preset.js (outside this file):
// jest.preset.js
module.exports = {
// ...
transformIgnorePatterns: [
'node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill|@cowprotocol|@uniswap)/)',
],
};Verify config consistency and allowJs:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check allowJs in tsconfig.spec.json =="
fd -a tsconfig.spec.json | while read -r f; do
echo "-> $f"
rg -n '"allowJs"\s*:\s*true' "$f" || echo "allowJs not set to true"
done
echo
echo "== transformIgnorePatterns occurrences (should be identical across projects) =="
rg -n --glob "**/jest.config.ts" -C2 "transformIgnorePatterns|node_modules/\(\?\!\("Length of output: 1511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== transformIgnorePatterns occurrences in jest.config.ts across repo =="
rg -n --glob "**/jest.config.ts" "transformIgnorePatterns" -C2 || echo "No matches found"
echo
echo "== transformIgnorePatterns occurrences in jest.preset.js =="
rg -n --glob "**/jest.preset.js" "transformIgnorePatterns" -C2 || echo "None in jest.preset.js"Length of output: 1395
Enable JS transpilation and centralize transformIgnorePatterns
- Add
"allowJs": trueto everytsconfig.spec.jsonso ts-jest can transpile the whitelisted JS (none currently set). - DRY: move the identical
transformIgnorePatternsfrom eachjest.config.ts(e.g. libs/services/jest.config.ts:9-11, libs/repositories/jest.config.ts:9-11, apps/notification-producer/jest.config.ts:9-11) intojest.preset.jsand remove the duplicates. - If any dependencies ship
.mjs, add a JS transformer (e.g.babel-jest) or configure ESM settings in Jest.
🤖 Prompt for AI Agents
In libs/repositories/jest.config.ts around lines 9 to 11, the
transformIgnorePatterns list is duplicated across multiple jest.config.ts files
and JS files aren't allowed for ts-jest transpilation; update every
tsconfig.spec.json in the repo to include "allowJs": true so ts-jest can
transpile whitelisted JS, remove the redundant transformIgnorePatterns from this
file and the other duplicate jest.config.ts files, and instead centralize that
pattern in jest.preset.js; additionally, if any dependency ships .mjs files, add
a JS transformer (e.g. babel-jest) to the preset or configure Jest's ESM
handling so .mjs modules are transformed correctly.
| async getTokenList( | ||
| chainId: SupportedChainId | ||
| ): Promise<TokenFromAPI[] | null> { | ||
| const key = this.getKey(chainId); | ||
| const hashData = await this.redisClient.hgetall(key); | ||
|
|
||
| if (!hashData || Object.keys(hashData).length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| const tokens: TokenFromAPI[] = []; | ||
|
|
||
| for (const [address, serializedToken] of Object.entries(hashData)) { | ||
| try { | ||
| const token = JSON.parse(serializedToken) as TokenFromAPI; | ||
| tokens.push(token); | ||
| } catch (error) { | ||
| await this.redisClient.hdel(key, address); | ||
| console.warn( | ||
| `Failed to parse token data for address ${address}, removed from cache` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return tokens.length > 0 ? tokens : null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Redis errors in getTokenList; log and return null instead of rejecting.
Currently, a rejected HGETALL will throw and break callers/tests. Wrap in try/catch and use the shared logger; also replace console.warn with logger.warn for consistency.
-import { injectable } from 'inversify';
+import { injectable } from 'inversify';
+import { logger } from '@cowprotocol/shared';
@@
- async getTokenList(
- chainId: SupportedChainId
- ): Promise<TokenFromAPI[] | null> {
- const key = this.getKey(chainId);
- const hashData = await this.redisClient.hgetall(key);
+ async getTokenList(
+ chainId: SupportedChainId
+ ): Promise<TokenFromAPI[] | null> {
+ const key = this.getKey(chainId);
+ let hashData: Record<string, string>;
+ try {
+ hashData = await this.redisClient.hgetall(key);
+ } catch (error) {
+ logger.error(error as Error, `[token-cache] hgetall failed for ${key}`);
+ return null;
+ }
@@
- } catch (error) {
- await this.redisClient.hdel(key, address);
- console.warn(
- `Failed to parse token data for address ${address}, removed from cache`
- );
+ } catch (error) {
+ await this.redisClient.hdel(key, address);
+ logger.warn(
+ `[token-cache] Failed to parse token data for address ${address}, removed from cache`
+ );
}📝 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.
| async getTokenList( | |
| chainId: SupportedChainId | |
| ): Promise<TokenFromAPI[] | null> { | |
| const key = this.getKey(chainId); | |
| const hashData = await this.redisClient.hgetall(key); | |
| if (!hashData || Object.keys(hashData).length === 0) { | |
| return null; | |
| } | |
| const tokens: TokenFromAPI[] = []; | |
| for (const [address, serializedToken] of Object.entries(hashData)) { | |
| try { | |
| const token = JSON.parse(serializedToken) as TokenFromAPI; | |
| tokens.push(token); | |
| } catch (error) { | |
| await this.redisClient.hdel(key, address); | |
| console.warn( | |
| `Failed to parse token data for address ${address}, removed from cache` | |
| ); | |
| } | |
| } | |
| return tokens.length > 0 ? tokens : null; | |
| } | |
| import { injectable } from 'inversify'; | |
| import { logger } from '@cowprotocol/shared'; | |
| @injectable() | |
| export class TokenCacheRepositoryRedis implements TokenCacheRepository { | |
| async getTokenList( | |
| chainId: SupportedChainId | |
| ): Promise<TokenFromAPI[] | null> { | |
| const key = this.getKey(chainId); | |
| // Wrap HGETALL in try/catch to handle Redis errors gracefully | |
| let hashData: Record<string, string>; | |
| try { | |
| hashData = await this.redisClient.hgetall(key); | |
| } catch (error) { | |
| logger.error(error as Error, `[token-cache] hgetall failed for ${key}`); | |
| return null; | |
| } | |
| if (!hashData || Object.keys(hashData).length === 0) { | |
| return null; | |
| } | |
| const tokens: TokenFromAPI[] = []; | |
| for (const [address, serializedToken] of Object.entries(hashData)) { | |
| try { | |
| const token = JSON.parse(serializedToken) as TokenFromAPI; | |
| tokens.push(token); | |
| } catch (error) { | |
| await this.redisClient.hdel(key, address); | |
| logger.warn( | |
| `[token-cache] Failed to parse token data for address ${address}, removed from cache` | |
| ); | |
| } | |
| } | |
| return tokens.length > 0 ? tokens : null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In libs/repositories/src/repos/TokenCacheRepository/TokenCacheRepositoryRedis.ts
around lines 42 to 67, wrap the redisClient.hgetall call in a try/catch so Redis
errors are caught, logged with the shared logger (use this.logger.error(...)
with the error and context) and return null instead of letting the error
propagate; also replace the console.warn inside the JSON.parse catch with
this.logger.warn(...) and include the parse error and address, and when deleting
the bad entry keep awaiting hdel but log any hdel failures via this.logger.warn
or .error so callers/tests no longer reject on Redis failures.
alfetopito
left a comment
alfetopito
left a comment
There was a problem hiding this comment.
A couple of nitpicks.
Also, please check rabbit comments, I found some of them relevant.
| type: 'array', | ||
| items: { | ||
| type: 'object', | ||
| required: ['chainId', 'address', 'name', 'symbol', 'decimals', 'logoURI'], |
There was a problem hiding this comment.
I'd say the logoURI is optional
| required: ['chainId', 'address', 'name', 'symbol', 'decimals', 'logoURI'], | |
| required: ['chainId', 'address', 'name', 'symbol', 'decimals'], |
| description: 'Number of decimal places for the token.', | ||
| type: 'integer', | ||
| minimum: 0, | ||
| maximum: 18, |
There was a problem hiding this comment.
The standard is 18, but it's actually not the max AFAICT.
https://ethereum.stackexchange.com/questions/118896/can-an-erc-20-have-more-than-18-decimals.
I'd remove it:
| maximum: 18, |
| await this.updateTokenList(); | ||
| })(); | ||
| }, | ||
| waitTimeMilliseconds: WAIT_TIME, |
There was a problem hiding this comment.
Nitpick: The update will trigger on every app start, right? We might want to have a better control by tracking in the db when it was last run.
I understand this is the hackathon and there's no need to address it now.
How about adding a TODO to address at some point in the future?
| networks: | ||
| - bff-network | ||
| depends_on: | ||
| - api |
There was a problem hiding this comment.
Isn't it the other way around? The API will benefit from/require the updater to run, but the updater doesn't need the api to run, no?
| const chainName = SUPPORTED_COINGECKO_PLATFORMS[chainId]; | ||
|
|
||
| if (!chainName) { | ||
| throw new Error(`Chain ${chainId} is not supported by CoinGecko`); | ||
| } | ||
|
|
||
| logger.info(`Initializing token list for chain ${chainId}`); | ||
| const tokens = await getTokensByChainName(chainName, chainId); |
There was a problem hiding this comment.
Nitpick: Another TODO: allow different sources/more token lists.
For example, the Gnosis chain token list from coingecko is list has 110 tokens (https://tokens.coingecko.com/xdai/all.json), while Honeyswap has 378 (https://tokens.honeyswap.org/)
| @injectable() | ||
| export class TokenCacheRepositoryRedis implements TokenCacheRepository { | ||
| private readonly keyPrefix = 'tokens:'; | ||
| private readonly defaultTtl = 24 * 60 * 60; // 24 hours in seconds |
There was a problem hiding this comment.
Nitpick: const outside the class?
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/docker.yml (2)
37-39: Broken reference to Buildx outputs (missing step id), builder input becomes emptydocker/build-push-action references steps.setup-buildx.outputs.name, but the setup-buildx step has no id, so outputs are unavailable. Add an id and keep the builder wiring.
Apply:
- - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + - name: Set up Docker Buildx + id: setup-buildx + uses: docker/setup-buildx-action@v3Also applies to: 56-66
26-30: matrix.node-version is undefined — setup-node will not get a versionThis likely breaks Node setup. Use a fixed LTS value here to avoid expanding the matrix.
Apply:
- - name: Use Node.js ${{ matrix.node-version }} + - name: Use Node.js 20.x uses: actions/setup-node@v3 with: - node-version: ${{ matrix.node-version }} + node-version: 20 + cache: 'yarn'
🧹 Nitpick comments (2)
.github/workflows/docker.yml (2)
61-63: Pushing images on pull_request may be undesirableCurrent config pushes to GHCR for PRs too. If you want to avoid registry churn or fork-permissions failures, gate pushes to non-PR events.
Example:
- push: true + push: ${{ github.event_name != 'pull_request' }}
35-36: Build scope can be tightened for speedBuilding the whole monorepo can be slow. Consider targeted Nx builds per package (with dependencies).
Example:
- - run: yarn build + - run: yarn nx build ${{ env.PACKAGE_NAME }} --with-deps
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: docker (apps/twap)
- GitHub Check: docker (apps/telegram)
- GitHub Check: docker (apps/notification-producer)
- GitHub Check: docker (apps/api)
- GitHub Check: docker (apps/token-list-updater)
🔇 Additional comments (1)
.github/workflows/docker.yml (1)
21-21: Approve matrix entry for token-list-updater Dockerfile exists at apps/token-list-updater/Dockerfile and project.json contains the correctnameandbuildconfiguration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/src/app/inversify.config.ts (2)
61-61: Guard repository creation: avoid crashing when Redis is unavailablegetTokenCacheRepository() can throw if Redis/env is missing. Wrap in try/catch and proceed without binding to preserve graceful startup. This mirrors prior feedback.
Apply:
- const tokenCacheRepository = getTokenCacheRepository(); + let tokenCacheRepository: TokenCacheRepository | undefined; + try { + tokenCacheRepository = getTokenCacheRepository(); + } catch (err) { + logger.warn('Token cache repository not initialized; proceeding without Redis', err); + }
96-98: Bind conditionally: only register TokenCacheRepository if createdUnconditionally binding will fail when creation is skipped/failed. Bind only when available; ensure consumers treat it as optional.
Apply:
- apiContainer - .bind<TokenCacheRepository>(tokenCacheRepositorySymbol) - .toConstantValue(tokenCacheRepository); + if (tokenCacheRepository) { + apiContainer + .bind<TokenCacheRepository>(tokenCacheRepositorySymbol) + .toConstantValue(tokenCacheRepository); + }Verify all injection sites can tolerate absence (use @optional()):
#!/bin/bash # Show TokenCacheRepository injection sites with context rg -nP -C3 '@inject\(\s*tokenCacheRepositorySymbol\s*\)' --type=ts # Show optional annotations nearby rg -nP -C2 '@optional\(\)' --type=ts
🧹 Nitpick comments (1)
apps/api/src/app/inversify.config.ts (1)
51-66: Reduce duplication: source a single token-cache instance via DIMain likely also creates/sets the token cache. Prefer a single source of truth: create/own the instance in DI and let main.ts consume it from the container (or have DI set it via setTokenCacheRepository(instance)) to avoid multiple instances and lifecycle skew.
If you want, I can draft a small follow-up PR plan to centralize creation and call setTokenCacheRepository() from a DI activation hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/app/inversify.config.ts(4 hunks)libs/repositories/src/index.ts(2 hunks)libs/services/src/factories.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/repositories/src/index.ts
- libs/services/src/factories.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: docker (apps/twap)
- GitHub Check: docker (apps/api)
- GitHub Check: docker (apps/telegram)
- GitHub Check: docker (apps/token-list-updater)
- GitHub Check: docker (apps/notification-producer)
🔇 Additional comments (2)
apps/api/src/app/inversify.config.ts (2)
7-7: LGTM: importing getTokenCacheRepositoryImport is correct and aligns with usage below.
40-42: LGTM: DI types/symbols for TokenCacheRepositorySymbols and types match the binding usage.
Summary by CodeRabbit
New Features
Tests
Chores