add svg route #3424
add svg route #3424
Conversation
trajan0x
requested review from
bigboydiamonds,
abtestingalpha and
Defi-Moses
as code owners
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces new dependencies to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/rest-api/src/routes/chainIconRoute.ts (1)
1-5: Consider using built-in fetch APIThe implementation looks good, but consider using the built-in
fetchAPI available in Node.js 18+ instead of thenode-fetchpackage to reduce dependencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/rest-api/package.json(2 hunks)packages/rest-api/src/routes/chainIconRoute.ts(1 hunks)packages/rest-api/src/routes/index.ts(2 hunks)
🔇 Additional comments (2)
packages/rest-api/package.json (1)
26-26: Verify version compatibility with rest-api
Let's ensure the version of @synapsecns/synapse-constants (^1.8.3) is compatible with the rest-api package version (1.8.9).
✅ Verification successful
Version compatibility verified between rest-api and synapse-constants
The version of @synapsecns/synapse-constants (^1.8.3) is perfectly aligned with the current version (1.8.3) of the package. Additionally, other packages in the monorepo (@synapsecns/explorer-ui and @synapsecns/bridge-docs) are using the same version range, indicating consistent version management across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version alignment between packages
# Check if synapse-constants is used in other packages
fd -t f package.json -x jq -r 'select(.dependencies."@synapsecns/synapse-constants" != null) | "\(.name): \(.dependencies."@synapsecns/synapse-constants")"'
# Check the latest version of synapse-constants
if [ -f "packages/synapse-constants/package.json" ]; then
jq -r '.version' packages/synapse-constants/package.json
fi
Length of output: 367
packages/rest-api/src/routes/index.ts (1)
14-14: Verify route naming convention
The implementation looks good and follows the existing pattern. However, let's verify if the route name chainIcon follows the project's naming convention, as other routes use camelCase for compound words (e.g., tokenList, bridgeTxStatus).
Also applies to: 29-29
✅ Verification successful
Route naming follows existing convention
The route name chainIcon follows the established camelCase naming convention in the codebase, consistent with other compound word routes like tokenList, bridgeTxStatus, bridgeTxInfo, destinationTokens, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check route naming patterns in the codebase
rg "router\.use\('/.+'" packages/rest-api/src/routes/
Length of output: 966
packages/rest-api/package.json
Outdated
| @@ -31,6 +32,7 @@ | |||
| "http-proxy-middleware": "^3.0.3", | |||
| "jest": "^29.7.0", | |||
| "lodash": "^4.17.21", | |||
| "node-fetch": "^3.3.2", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing node-fetch dependency
Since the project requires Node.js >=18.17.0 (which has built-in fetch API), we could remove the node-fetch dependency and use the native fetch instead. This would reduce the package size and simplify dependencies.
- "node-fetch": "^3.3.2",| } catch (error) { | ||
| console.error('Error fetching chain icon:', error) | ||
| res.status(500).json({ error: 'Failed to fetch chain icon' }) | ||
| } | ||
| return | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging
The error handling could be enhanced:
- The
returnstatement after try-catch is unnecessary - Error logging should be structured and sanitized
- Different error types should have different status codes
Apply these improvements:
- } catch (error) {
- console.error('Error fetching chain icon:', error)
- res.status(500).json({ error: 'Failed to fetch chain icon' })
- }
- return
+ } catch (error) {
+ // Sanitize error message
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error'
+
+ // Log structured error
+ console.error({
+ error: 'Failed to fetch chain icon',
+ chainId,
+ message: errorMessage,
+ timestamp: new Date().toISOString()
+ })
+
+ // Return appropriate status code
+ if (errorMessage.includes('Invalid URL') || errorMessage.includes('Invalid content type')) {
+ res.status(400).json({ error: 'Invalid chain icon configuration' })
+ } else if (error instanceof Error && error.name === 'AbortError') {
+ res.status(504).json({ error: 'Chain icon fetch timeout' })
+ } else {
+ res.status(500).json({ error: 'Failed to fetch chain icon' })
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| router.get('/:chainId.svg', async (req, res) => { | ||
| const chainId = parseInt(req.params.chainId, 10) | ||
|
|
||
| // Find the chain with matching ID | ||
| const chain = Object.values(CHAINS).find( | ||
| (c): c is Chain => | ||
| typeof c === 'object' && c !== null && 'id' in c && c.id === chainId, | ||
| ) | ||
|
|
||
| if (!chain || !chain.chainImg) { | ||
| res.status(404).json({ error: 'Chain icon not found' }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input validation and type safety
While the chain validation is good, consider these improvements:
- Add input validation for chainId format before parsing
- Replace the type assertion with a proper type guard function
Consider this implementation:
router.get('/:chainId.svg', async (req, res) => {
+ if (!/^\d+$/.test(req.params.chainId)) {
+ res.status(400).json({ error: 'Invalid chain ID format' })
+ return
+ }
const chainId = parseInt(req.params.chainId, 10)
- // Find the chain with matching ID
- const chain = Object.values(CHAINS).find(
- (c): c is Chain =>
- typeof c === 'object' && c !== null && 'id' in c && c.id === chainId,
- )
+ const isChain = (c: unknown): c is Chain =>
+ typeof c === 'object' && c !== null && 'id' in c && 'chainImg' in c
+
+ const chain = Object.values(CHAINS).find(isChain)Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| // Fetch the image from the URL | ||
| const response = await fetch(chain.chainImg) | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch image: ${response.statusText}`) | ||
| } | ||
|
|
||
| const buffer = await response.buffer() | ||
|
|
||
| // Set cache headers (cache for 1 week) | ||
| res.set({ | ||
| 'Cache-Control': 'public, max-age=604800', | ||
| 'Content-Type': response.headers.get('content-type') || 'image/svg+xml', | ||
| }) | ||
|
|
||
| res.send(buffer) | ||
| } catch (error) { |
There was a problem hiding this comment.
Address security concerns in image fetching
Several security issues need to be addressed:
- No validation of the image URL
- No timeout on fetch requests
- No size limits on responses
- No validation of content type
Apply these security improvements:
try {
+ // Validate URL
+ const imageUrl = new URL(chain.chainImg)
+ if (!['http:', 'https:'].includes(imageUrl.protocol)) {
+ throw new Error('Invalid URL protocol')
+ }
+
// Fetch the image from the URL
- const response = await fetch(chain.chainImg)
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), 5000)
+
+ const response = await fetch(imageUrl, {
+ signal: controller.signal,
+ headers: {
+ 'Accept': 'image/svg+xml'
+ }
+ })
+ clearTimeout(timeout)
+
if (!response.ok) {
throw new Error(`Failed to fetch image: ${response.statusText}`)
}
+ const contentType = response.headers.get('content-type')
+ if (!contentType?.startsWith('image/svg+xml')) {
+ throw new Error('Invalid content type')
+ }
+
+ // Limit response size to 1MB
+ const contentLength = parseInt(response.headers.get('content-length') ?? '0', 10)
+ if (contentLength > 1024 * 1024) {
+ throw new Error('Image too large')
+ }
+
const buffer = await response.buffer()
// Set cache headers (cache for 1 week)
res.set({
'Cache-Control': 'public, max-age=604800',
- 'Content-Type': response.headers.get('content-type') || 'image/svg+xml',
+ 'Content-Type': 'image/svg+xml',
})📝 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 { | |
| // Fetch the image from the URL | |
| const response = await fetch(chain.chainImg) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch image: ${response.statusText}`) | |
| } | |
| const buffer = await response.buffer() | |
| // Set cache headers (cache for 1 week) | |
| res.set({ | |
| 'Cache-Control': 'public, max-age=604800', | |
| 'Content-Type': response.headers.get('content-type') || 'image/svg+xml', | |
| }) | |
| res.send(buffer) | |
| } catch (error) { | |
| try { | |
| // Validate URL | |
| const imageUrl = new URL(chain.chainImg) | |
| if (!['http:', 'https:'].includes(imageUrl.protocol)) { | |
| throw new Error('Invalid URL protocol') | |
| } | |
| // Fetch the image from the URL | |
| const controller = new AbortController() | |
| const timeout = setTimeout(() => controller.abort(), 5000) | |
| const response = await fetch(imageUrl, { | |
| signal: controller.signal, | |
| headers: { | |
| 'Accept': 'image/svg+xml' | |
| } | |
| }) | |
| clearTimeout(timeout) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch image: ${response.statusText}`) | |
| } | |
| const contentType = response.headers.get('content-type') | |
| if (!contentType?.startsWith('image/svg+xml')) { | |
| throw new Error('Invalid content type') | |
| } | |
| // Limit response size to 1MB | |
| const contentLength = parseInt(response.headers.get('content-length') ?? '0', 10) | |
| if (contentLength > 1024 * 1024) { | |
| throw new Error('Image too large') | |
| } | |
| const buffer = await response.buffer() | |
| // Set cache headers (cache for 1 week) | |
| res.set({ | |
| 'Cache-Control': 'public, max-age=604800', | |
| 'Content-Type': 'image/svg+xml', | |
| }) | |
| res.send(buffer) | |
| } catch (error) { |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3424 +/- ##
============================================
Coverage ? 90.44834%
============================================
Files ? 54
Lines ? 1026
Branches ? 82
============================================
Hits ? 928
Misses ? 95
Partials ? 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
4883033
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7f2ae878.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-router.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/rest-api/src/routes/addressIconRoute.ts (3)
1-5: Consider adding Express type imports for better type safetyImport Request and Response types from express to improve type safety in the route handler.
-import express from 'express' +import express, { Request, Response } from 'express'
24-28: Improve error logging structureThe current logging could be more structured and informative.
- console.log('Token not found or no icon:', { token }) + console.log('Token icon not found', { + chainId, + address, + tokenFound: !!token, + hasIcon: token?.icon !== undefined + })
1-53: Consider implementing a CDN and caching strategyTo improve performance and reliability:
- Consider using a CDN to serve cached icons
- Implement a local cache to reduce external requests
- Add fallback icons for failed requests
This would reduce latency and dependencies on external services. Would you like assistance in implementing these improvements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/rest-api/src/routes/addressIconRoute.ts(1 hunks)packages/rest-api/src/routes/chainIconRoute.ts(1 hunks)packages/rest-api/src/routes/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/src/routes/index.ts
- packages/rest-api/src/routes/chainIconRoute.ts
| router.get('/:chainId/:address.svg', async (req, res) => { | ||
| const chainId = parseInt(req.params.chainId, 10) | ||
| const address = req.params.address.toLowerCase() |
There was a problem hiding this comment.
Add input validation for chainId and address parameters
The current implementation lacks validation for:
- chainId (could be NaN)
- address format (should be a valid hex string)
This could lead to unnecessary processing of invalid requests.
router.get('/:chainId/:address.svg', async (req, res) => {
const chainId = parseInt(req.params.chainId, 10)
+ if (isNaN(chainId)) {
+ res.status(400).json({ error: 'Invalid chainId' })
+ return
+ }
const address = req.params.address.toLowerCase()
+ if (!/^0x[a-f0-9]{40}$/i.test(address)) {
+ res.status(400).json({ error: 'Invalid address format' })
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.get('/:chainId/:address.svg', async (req, res) => { | |
| const chainId = parseInt(req.params.chainId, 10) | |
| const address = req.params.address.toLowerCase() | |
| router.get('/:chainId/:address.svg', async (req, res) => { | |
| const chainId = parseInt(req.params.chainId, 10) | |
| if (isNaN(chainId)) { | |
| res.status(400).json({ error: 'Invalid chainId' }) | |
| return | |
| } | |
| const address = req.params.address.toLowerCase() | |
| if (!/^0x[a-f0-9]{40}$/i.test(address)) { | |
| res.status(400).json({ error: 'Invalid address format' }) | |
| return | |
| } |
| // Find the token with matching address on the specified chain | ||
| const token = Object.values(BRIDGABLE_TOKENS[chainId] || []).find( | ||
| (t): t is Token => | ||
| typeof t === 'object' && | ||
| t !== null && | ||
| 'addresses' in t && | ||
| Object.entries(t.addresses).some(([chain, addr]) => { | ||
| const matches = | ||
| parseInt(chain, 10) === chainId && addr.toLowerCase() === address | ||
| return matches | ||
| }) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract token lookup logic and optimize performance
The token lookup logic is complex and could benefit from:
- Extraction into a separate function for better maintainability
- Direct object lookup instead of array operations
+const findToken = (chainId: number, address: string): Token | undefined => {
+ const chainTokens = BRIDGABLE_TOKENS[chainId]
+ if (!chainTokens) return undefined
+
+ return Object.values(chainTokens).find(
+ (t): t is Token =>
+ typeof t === 'object' &&
+ t !== null &&
+ 'addresses' in t &&
+ Object.entries(t.addresses).some(([chain, addr]) =>
+ parseInt(chain, 10) === chainId && addr.toLowerCase() === address
+ )
+ )
+}
+
router.get('/:chainId/:address.svg', async (req, res) => {
const chainId = parseInt(req.params.chainId, 10)
const address = req.params.address.toLowerCase()
- const token = Object.values(BRIDGABLE_TOKENS[chainId] || []).find(
- (t): t is Token =>
- typeof t === 'object' &&
- t !== null &&
- 'addresses' in t &&
- Object.entries(t.addresses).some(([chain, addr]) => {
- const matches =
- parseInt(chain, 10) === chainId && addr.toLowerCase() === address
- return matches
- })
- )
+ const token = findToken(chainId, address)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Find the token with matching address on the specified chain | |
| const token = Object.values(BRIDGABLE_TOKENS[chainId] || []).find( | |
| (t): t is Token => | |
| typeof t === 'object' && | |
| t !== null && | |
| 'addresses' in t && | |
| Object.entries(t.addresses).some(([chain, addr]) => { | |
| const matches = | |
| parseInt(chain, 10) === chainId && addr.toLowerCase() === address | |
| return matches | |
| }) | |
| ) | |
| const findToken = (chainId: number, address: string): Token | undefined => { | |
| const chainTokens = BRIDGABLE_TOKENS[chainId] | |
| if (!chainTokens) return undefined | |
| return Object.values(chainTokens).find( | |
| (t): t is Token => | |
| typeof t === 'object' && | |
| t !== null && | |
| 'addresses' in t && | |
| Object.entries(t.addresses).some(([chain, addr]) => | |
| parseInt(chain, 10) === chainId && addr.toLowerCase() === address | |
| ) | |
| ) | |
| } | |
| // Find the token with matching address on the specified chain | |
| const token = findToken(chainId, address) |
| try { | ||
| // Fetch the image from the URL | ||
| const response = await fetch(token.icon) | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch image: ${response.statusText}`) | ||
| } | ||
|
|
||
| const buffer = await response.arrayBuffer() | ||
|
|
||
| // Set cache headers (cache for 1 week) | ||
| res.set({ | ||
| 'Cache-Control': 'public, max-age=604800', | ||
| 'Content-Type': response.headers.get('content-type') || 'image/svg+xml', | ||
| }) | ||
|
|
||
| res.send(Buffer.from(buffer)) | ||
| } catch (error) { | ||
| console.error('Error fetching token icon:', error) | ||
| res.status(500).json({ error: 'Failed to fetch token icon' }) | ||
| } | ||
| return | ||
| }) |
There was a problem hiding this comment.
Add timeout and security measures for external requests
The external fetch operation needs additional safety measures:
- Request timeout
- URL validation
- Response size limits
+const isValidIconUrl = (url: string): boolean => {
+ try {
+ const parsed = new URL(url)
+ return ['http:', 'https:'].includes(parsed.protocol)
+ } catch {
+ return false
+ }
+}
try {
+ if (!isValidIconUrl(token.icon)) {
+ throw new Error('Invalid icon URL')
+ }
+
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), 5000)
+
- const response = await fetch(token.icon)
+ const response = await fetch(token.icon, {
+ signal: controller.signal,
+ headers: {
+ 'Accept': 'image/svg+xml,image/*'
+ }
+ })
+ clearTimeout(timeout)
if (!response.ok) {
throw new Error(`Failed to fetch image: ${response.statusText}`)
}
+ const contentLength = response.headers.get('content-length')
+ if (contentLength && parseInt(contentLength, 10) > 1024 * 1024) {
+ throw new Error('Icon file too large')
+ }
const buffer = await response.arrayBuffer()📝 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 { | |
| // Fetch the image from the URL | |
| const response = await fetch(token.icon) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch image: ${response.statusText}`) | |
| } | |
| const buffer = await response.arrayBuffer() | |
| // Set cache headers (cache for 1 week) | |
| res.set({ | |
| 'Cache-Control': 'public, max-age=604800', | |
| 'Content-Type': response.headers.get('content-type') || 'image/svg+xml', | |
| }) | |
| res.send(Buffer.from(buffer)) | |
| } catch (error) { | |
| console.error('Error fetching token icon:', error) | |
| res.status(500).json({ error: 'Failed to fetch token icon' }) | |
| } | |
| return | |
| }) | |
| try { | |
| if (!isValidIconUrl(token.icon)) { | |
| throw new Error('Invalid icon URL') | |
| } | |
| const controller = new AbortController() | |
| const timeout = setTimeout(() => controller.abort(), 5000) | |
| const response = await fetch(token.icon, { | |
| signal: controller.signal, | |
| headers: { | |
| 'Accept': 'image/svg+xml,image/*' | |
| } | |
| }) | |
| clearTimeout(timeout) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch image: ${response.statusText}`) | |
| } | |
| const contentLength = response.headers.get('content-length') | |
| if (contentLength && parseInt(contentLength, 10) > 1024 * 1024) { | |
| throw new Error('Icon file too large') | |
| } | |
| const buffer = await response.arrayBuffer() | |
| // Set cache headers (cache for 1 week) | |
| res.set({ | |
| 'Cache-Control': 'public, max-age=604800', | |
| 'Content-Type': response.headers.get('content-type') || 'image/svg+xml', | |
| }) | |
| res.send(Buffer.from(buffer)) | |
| } catch (error) { | |
| console.error('Error fetching token icon:', error) | |
| res.status(500).json({ error: 'Failed to fetch token icon' }) | |
| } | |
| return | |
| }) |
Bundle ReportChanges will increase total bundle size by 358.28kB (1.1%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Description
Adds an svg route
Summary by CodeRabbit
chainIdandaddress.@synapsecns/synapse-constantsandcross-fetchto enhance functionality.