feat(sdk-router): Gas.zip module [SYN-37] #3528
Conversation
WalkthroughThis pull request integrates comprehensive Gas.zip support into the SDK. New modules in the Gas.zip directory encapsulate API interactions, module classes, and module sets for bridging operations. In addition, the RFQ API and router components have been updated with improved error handling using a structured logger and a dedicated HTTP timeout utility. Enhancements to SDK utilities, token and chain configurations, and transaction status handling—especially for refunded transactions—are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GasZipModule
participant GasZipAPI
Client->>GasZipModule: bridge(to, destChainId, token, amount, originQuery, destQuery)
alt Native Token Check
Note right of GasZipModule: Return simple transaction if native token
else
GasZipModule->>GasZipAPI: getGasZipQuote(...parameters...)
GasZipAPI-->>GasZipModule: GasZipQuote response
GasZipModule->>Client: Return transaction with gas address & calldata
end
Client->>GasZipModule: getBridgeTxStatus(txHash)
GasZipModule->>GasZipAPI: getGasZipTxStatus(txHash)
GasZipAPI-->>GasZipModule: Transaction status (confirmed/refunded)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 th 8000 e PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/sdk-router/src/rfq/api.ts (1)
18-44: 🛠️ Refactor suggestionEnhance error handling and recovery.
The current implementation silently fails by returning an empty array. Consider:
- Adding retry mechanism for transient failures
- Distinguishing between different types of errors
- Providing more context in error responses
export const getAllQuotes = async (): Promise<FastBridgeQuote[]> => { + const MAX_RETRIES = 3 + const RETRY_DELAY = 500 // ms + + const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)) + + for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { try { const response = await getWithTimeout( 'RFQ API', `${API_URL}/quotes`, API_TIMEOUT ) if (!response) { + if (attempt === MAX_RETRIES) { + logger.error({ attempt }, 'Failed to get response after all retries') return [] + } + logger.warn({ attempt }, 'No response, retrying...') + await sleep(RETRY_DELAY) + continue } // The response is a list of quotes in the FastBridgeQuoteAPI format const quotes: FastBridgeQuoteAPI[] = await response.json() return quotes .map((quote) => { try { return unmarshallFastBridgeQuote(quote) } catch (error) { - logger.error({ quote, error }, 'Could not unmarshall quote') + logger.error({ + quote, + error, + attempt + }, 'Could not unmarshall quote') return null } }) .filter((quote): quote is FastBridgeQuote => quote !== null) } catch (error) { - logger.error({ error }, 'Failed to fetch all quotes') + if (attempt === MAX_RETRIES) { + logger.error({ + error, + attempt, + url: `${API_URL}/quotes`, + timeout: API_TIMEOUT + }, 'Failed to fetch quotes after all retries') + return [] + } + logger.warn({ error, attempt }, 'Request failed, retrying...') + await sleep(RETRY_DELAY) + continue + } + } return [] - } }packages/sdk-router/src/utils/RouterCache.ts (1)
1-2:⚠️ Potential issueRemove TypeScript ignores and fix type issues.
Using
@ts-nocheckand@ts-ignoresuppresses potential type issues that should be addressed properly.
🧹 Nitpick comments (10)
packages/sdk-router/src/utils/addressUtils.ts (1)
1-3: Consider adding input validation and documentation.While the function correctly handles case-insensitive address comparison, consider these improvements:
- Add JSDoc comments to document the purpose and parameters.
- Add validation for address format (e.g., check if they are valid Ethereum addresses).
+/** + * Compares two addresses case-insensitively. + * @param addr1 First address to compare + * @param addr2 Second address to compare + * @returns true if both addresses are the same (case-insensitive), false otherwise + */ export const isSameAddress = (addr1?: string, addr2?: string): boolean => { + const isValidAddress = (addr?: string) => { + return addr?.match(/^0x[a-fA-F0-9]{40}$/) !== null + } + if (!addr1 || !addr2 || !isValidAddress(addr1) || !isValidAddress(addr2)) { + return false + } return !!addr1 && !!addr2 && addr1.toLowerCase() === addr2.toLowerCase() }packages/sdk-router/src/utils/logger.ts (1)
3-18: Improve execution time logging accuracy and security.The current implementation has several areas for improvement:
Date.now()is not as precise asperformance.now()for timing measurements- Arguments are logged without sanitization, which could leak sensitive data
- The decorator lacks TypeScript type safety
// Decorator to log the execution time of a function export const logExecutionTime = (functionName: string) => - (_target: any, _propertyKey: string, descriptor: PropertyDescriptor) => { + <T>(_target: any, _propertyKey: string, descriptor: TypedPropertyDescriptor<T>) => { const originalMethod = descriptor.value descriptor.value = async function (...args: any[]) { - const startTime = Date.now() + const startTime = performance.now() const result = await originalMethod.apply(this, args) - const elapsedTime = Date.now() - startTime - logger.info({ args }, `${functionName} execution time: ${elapsedTime}ms`) + const elapsedTime = performance.now() - startTime + // Sanitize arguments to prevent sensitive data logging + const sanitizedArgs = args.map(arg => + typeof arg === 'object' ? '[Object]' : + typeof arg === 'function' ? '[Function]' : + arg + ) + logger.info( + { args: sanitizedArgs }, + `${functionName} execution time: ${elapsedTime.toFixed(2)}ms` + ) return result } return descriptor }packages/sdk-router/src/rfq/api.ts (1)
9-10: Consider making API configuration more flexible.Hard-coded values for API URL and timeout should be configurable:
- Move to environment variables or configuration object
- Allow different timeouts for different environments
-const API_URL = 'https://rfq-api.omnirpc.io' -const API_TIMEOUT = 2000 +const API_URL = process.env.RFQ_API_URL || 'https://rfq-api.omnirpc.io' +const API_TIMEOUT = parseInt(process.env.RFQ_API_TIMEOUT || '2000', 10)packages/sdk-router/src/utils/RouterCache.ts (1)
39-77: Consider adding cache invalidation and error recovery.The current implementation lacks:
- Cache invalidation mechanism for failed operations
- Error recovery strategy
- Type safety for cached values
packages/sdk-router/src/gaszip/gasZipModule.ts (1)
11-12: Consider moving the hardcoded address to a configuration file.The contract address should be moved to a constants or configuration file to improve maintainability and allow for easier updates.
- readonly address = '0x391E7C679d29bD940d63be94AD22A25d25b5A604' + readonly address = GAS_ZIP_CONTRACT_ADDRESSpackages/sdk-router/src/utils/api.ts (1)
3-39: Consider adding retry mechanism for transient failures.The fetchWithTimeout implementation handles timeouts well but could benefit from a retry mechanism for transient failures.
const fetchWithTimeout = async ( name: string, url: string, timeout: number, params: any = {}, init?: RequestInit + retries: number = 3, + backoff: number = 1000, ): Promise<Response | null> => { const controller = new AbortController() const timeoutId = setTimeout(() => controller.abort(), timeout) + let attempt = 0 + while (attempt < retries) { try { const response = await fetch(url, { signal: controller.signal, ...init, }) if (!response.ok) { const text = await response.text() logger.info( { name, url, params, response, text }, `${name}: response not OK` ) + if (attempt < retries - 1) { + await new Promise(resolve => setTimeout(resolve, backoff * Math.pow(2, attempt))) + attempt++ + continue + } return null } return response } catch (error) { if (error instanceof Error && error.name === 'AbortError') { logger.info({ name, url, timeout, params }, `${name}: timed out`) } else { logger.error( { name, url, params, error }, `${name}: was not able to get a response` ) } + if (attempt < retries - 1) { + await new Promise(resolve => setTimeout(resolve, backoff * Math.pow(2, attempt))) + attempt++ + continue + } return null } finally { clearTimeout(timeoutId) } + } + return null }packages/sdk-router/src/gaszip/api.ts (1)
7-8: Consider making API configuration configurable.The API URL and timeout should be configurable through environment variables or configuration files.
-const GAS_ZIP_API_URL = 'https://backend.gas.zip/v2' -const GAS_ZIP_API_TIMEOUT = 2000 +const GAS_ZIP_API_URL = process.env.GAS_ZIP_API_URL || 'https://backend.gas.zip/v2' +const GAS_ZIP_API_TIMEOUT = Number(process.env.GAS_ZIP_API_TIMEOUT) || 2000packages/sdk-router/src/gaszip/gasZipModuleSet.ts (3)
22-44: Consider adding type safety for chainId.The modules and providers objects use number as the key type. Consider using a more specific type like
ChainIdto ensure type safety.+type ChainId = number; + export class GasZipModuleSet extends SynapseModuleSet { public readonly bridgeModuleName = 'Gas.zip' public readonly allEvents = [] public modules: { - [chainId: number]: GasZipModule + [chainId in ChainId]: GasZipModule } public providers: { - [chainId: number]: Provider + [chainId in ChainId]: Provider }
124-137: Implement fee data calculation.The getFeeData method currently returns zero values. Consider implementing proper fee calculation based on Gas.zip's pricing model.
Would you like me to help implement the fee calculation logic based on Gas.zip's documentation?
142-151: Implement period settings.The getDefaultPeriods method returns zero values. Consider implementing appropriate period settings for Gas.zip operations.
Would you like me to help implement the period settings based on Gas.zip's requirements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/sdk-router/src/gaszip/api.ts(1 hunks)packages/sdk-router/src/gaszip/gasZipModule.ts(1 hunks)packages/sdk-router/src/gaszip/gasZipModuleSet.ts(1 hunks)packages/sdk-router/src/gaszip/index.ts(1 hunks)packages/sdk-router/src/rfq/api.ts(3 hunks)packages/sdk-router/src/router/router.ts(3 hunks)packages/sdk-router/src/router/routerSet.ts(2 hunks)packages/sdk-router/src/sdk.ts(3 hunks)packages/sdk-router/src/utils/RouterCache.ts(2 hunks)packages/sdk-router/src/utils/addressUtils.test.ts(1 hunks)packages/sdk-router/src/utils/addressUtils.ts(1 hunks)packages/sdk-router/src/utils/api.ts(1 hunks)packages/sdk-router/src/utils/logger.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk-router/src/gaszip/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: changesets-integrity-checker
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (8)
packages/sdk-router/src/gaszip/gasZipModule.ts (2)
17-22: LGTM! Good use of invariant checks.The constructor properly validates both chainId and provider using invariant checks.
59-68: LGTM! Simple and effective transaction status handling.The implementation correctly delegates to the API layer for transaction status checks.
packages/sdk-router/src/utils/api.ts (1)
41-89: LGTM! Well-structured HTTP method wrappers.The implementation provides clean wrappers for GET, POST, and PUT methods with proper parameter handling.
packages/sdk-router/src/sdk.ts (1)
21-21: LGTM! Clean integration of GasZipModuleSet.The Gas.zip module is properly integrated into the SDK with appropriate initialization in the constructor.
Also applies to: 50-51, 55-55
packages/sdk-router/src/utils/addressUtils.test.ts (1)
3-114: Well-organized test suite with comprehensive coverage!The test suite effectively covers all important scenarios for Ethereum address comparison:
- Different address formats (lowercase, checksummed, uppercase)
- Different addresses
- Edge cases (undefined, empty, null)
packages/sdk-router/src/router/router.ts (2)
121-124: Good improvement in error logging structure!The structured logging with context parameters improves error traceability.
145-148: Good improvement in error logging structure!The structured logging with context parameters improves error traceability.
packages/sdk-router/src/router/routerSet.ts (1)
144-155: Excellent improvement in error logging structure!The structured logging now includes comprehensive context (chainIds, tokens, amounts, etc.) which will greatly help with debugging issues.
Deploying sanguine-fe with
|
| Latest commit: |
29910be
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6f1f1264.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-gas-zip-module.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/synapse-interface/slices/bridgeQuote/thunks.ts (1)
43-52: 💡 Verification agent❓ Verification inconclusive
Consider adding error handling for Gas.zip API failures.
The
allBridgeQuotescall should handle potential Gas.zip API failures gracefully to ensure the system can fall back to non-Gas.zip quotes when necessary.
🏁 Script executed:
#!/bin/bash # Description: Check error handling in Gas.zip API integration # Look for error handling in Gas.zip API calls rg -A 5 "catch|throw|Error" "packages/sdk-router/src/gaszip/api.ts" # Check for retry mechanisms rg -A 5 "retry|backoff" "packages/sdk-router/src/gaszip/"Length of output: 125
Action Required: Add Error Handling for Gas.zip API Failures
The call to
synapseSDK.allBridgeQuotesinpackages/synapse-interface/slices/bridgeQuote/thunks.ts(lines 43–52) does not appear to be wrapped in any error handling, and our search for error handling keywords in the Gas.zip integration module (i.e. inpackages/sdk-router/src/gaszip/api.tsand its related files) returned no results. To ensure the system can gracefully fall back to non-Gas.zip quotes when the Gas.zip API fails, please consider wrapping the API call in a try-catch block and, if applicable, implement a retry/backoff mechanism.
- Location:
packages/synapse-interface/slices/bridgeQuote/thunks.ts(lines 43–52)- Suggestion: Enclose the
synapseSDK.allBridgeQuotescall in a try-catch block to catch API-related errors, log them, and initiate a fallback to non-Gas.zip quotes.
🧹 Nitpick comments (2)
packages/synapse-interface/slices/bridgeQuote/thunks.ts (2)
91-102: Update analytics event name for consistency.The analytics event name "[Bridge] use non-RFQ quote over RFQ" should be updated to reflect the new Gas.zip terminology.
- segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, { + segmentAnalyticsEvent(`[Bridge] use non-Gas.zip quote over Gas.zip`, {
82-86: Consider making the allowed percentile difference configurable.The hardcoded 30% difference threshold for quote comparison could be made configurable to allow for easier adjustments based on market conditions or Gas.zip performance metrics.
- const allowedPercentileDifference = 30n + const allowedPercentileDifference = QUOTE_COMPARISON_THRESHOLD_PERCENTConsider adding this constant to a configuration file:
// config/bridge.ts export const QUOTE_COMPARISON_THRESHOLD_PERCENT = 30n
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/synapse-interface/slices/bridgeQuote/thunks.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: SonarCloud
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/synapse-interface/slices/bridgeQuote/thunks.ts (1)
70-70:❓ Verification inconclusive
Verify the Gas.zip module integration.
The quote selection logic has been updated to use 'Gas.zip' instead of 'SynapseRFQ'. This change appears to be part of the Gas.zip module integration, but there are some considerations to verify:
- Ensure that the Gas.zip module is properly initialized in the SDK before these quotes are requested.
- Verify that the Gas.zip quotes follow the same interface as the previous RFQ quotes.
Also applies to: 72-74
🏁 Script executed:
#!/bin/bash # Description: Verify Gas.zip module initialization and quote interface # Check for Gas.zip module initialization rg -A 5 "Gas\.zip" "packages/sdk-router/src/sdk.ts" # Check quote interface implementation ast-grep --pattern $'class GasZipModule { $$$ getQuote($$$) { $$$ } $$$ }'Length of output: 139
Attention: Verify the Gas.zip Integration Details
The quote selection logic now uses
'Gas.zip'instead of'SynapseRFQ', which aligns with the intended integration of the Gas.zip module. However, the initial verification script did not return any output, so please ensure that:
- The Gas.zip module is properly initialized within the SDK (for example, in
packages/sdk-router/src/sdk.tsor another central configuration file).- The Gas.zip quotes conform to the expected interface (especially matching what downstream logic consumes, such as fee calculations, slippage, and analytics tracking).
I recommend running additional verification commands to search the repository for references to
"Gas.zip"and a possibleGasZipModuleclass implementation, as follows:Please review the output of these commands to confirm that the Gas.zip module is both initialized and behaves as expected. If no references are found, manual verification of the module initialization and interface is needed.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for 'Gas.zip' references in the repository:" rg "Gas\.zip" echo "Searching for the class definition of GasZipModule:" rg "class GasZipModule" -n -C 5 . echo "Searching for instances of the getQuote method:" rg "getQuote\(" -n -C 5 .Length of output: 2935
Verify Gas.zip Integration and Quote Interface
The quote selection logic now opts for
'Gas.zip'instead of'SynapseRFQ', aligning with the new Gas.zip integration. Based on our search:
- References to
'Gas.zip'are confirmed in the code (e.g., inpackages/synapse-interface/slices/bridgeQuote/thunks.ts).- The Gas.zip module is initialized in the SDK, with related definitions in
packages/sdk-router/src/gaszip/gasZipModuleSet.tsandpackages/sdk-router/src/gaszip/gasZipModule.ts.- The expected
getQuotemethod wasn’t directly found; if the Gas.zip quote interface is implemented via another function (such asgetGasZipQuote), please verify that its behavior fully aligns with the previous RFQ quote interface — especially concerning fee calculations, slippage, and analytics processing.Please manually verify the initialization and interface compliance to ensure a smooth transition with the new Gas.zip module.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/synapse-interface/components/StateManagedBridge/BridgeExchangeRateInfo.tsx (2)
66-66: Enhance the TODO comment with more details.The TODO comment about handling slippage between different types of assets would be more actionable if it included specific examples or scenarios that need to be addressed.
Consider expanding the comment like this:
- // TODO: handle slippage between different types of assets + // TODO: handle slippage between different types of assets (e.g., stable/non-stable pairs, + // tokens with different decimal places, or wrapped/unwrapped versions)
76-76: Document why slippage is hidden for Gas.zip.The code hides slippage when
bridgeModuleNameis 'Gas.zip', but there's no explanation for this behavior. Please add a comment explaining why slippage is not applicable or should be hidden for Gas.zip transactions.Consider refactoring the complex condition for better readability:
- {safeFromAmount !== '0' && !underFee && bridgeModuleName !== 'Gas.zip' ? ( + {shouldShowSlippage(safeFromAmount, underFee, bridgeModuleName) ? (And add a helper function with documentation:
/** * Determines whether to show slippage based on the transaction parameters. * Slippage is hidden for: * - Zero amount transactions * - Transactions under fee * - Gas.zip transactions (because ...) */ const shouldShowSlippage = ( amount: string, underFee: boolean, moduleName?: string ): boolean => { return amount !== '0' && !underFee && moduleName !== 'Gas.zip' }packages/synapse-interface/scripts/utils/fetchGasZipData.js (1)
1-1: Move API URL to environment configuration.The Gas.zip API URL should be loaded from environment variables to support different environments and improve maintainability.
-const GAS_ZIP_URL = 'https://backend.gas.zip/v2/chains' +const GAS_ZIP_URL = process.env.GAS_ZIP_URL || 'https://backend.gas.zip/v2/chains'packages/synapse-interface/scripts/generateMaps.js (1)
266-273: OptimizegetGasZipOriginMapimplementation.The function creates a new object and set for each call. For better performance, consider caching the result for repeated calls with the same chainId.
+const gasZipOriginMapCache = new Map() + const getGasZipOriginMap = async (chainId, gasZipChains) => { if (!gasZipChains.includes(Number(chainId))) { return {} } + const cacheKey = `${chainId}` + if (gasZipOriginMapCache.has(cacheKey)) { + return gasZipOriginMapCache.get(cacheKey) + } const tokensToSymbols = {} tokensToSymbols[ETH] = new Set(['Gas.zip']) + gasZipOriginMapCache.set(cacheKey, tokensToSymbols) return tokensToSymbols }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/synapse-interface/components/StateManagedBridge/BridgeExchangeRateInfo.tsx(1 hunks)packages/synapse-interface/constants/bridgeMap.ts(28 hunks)packages/synapse-interface/scripts/generateMaps.js(5 hunks)packages/synapse-interface/scripts/utils/fetchGasZipData.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: SonarCloud
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (3)
packages/synapse-interface/scripts/generateMaps.js (1)
399-403: LGTM! Clean integration of Gas.zip origin map.The integration with the global origin map is well-implemented and maintains consistency with other bridge types.
packages/synapse-interface/constants/bridgeMap.ts (2)
467-473: LGTM! Consistent Gas.zip integration across chains.The Gas.zip integration follows a consistent pattern across all supported chains, with proper origin and destination mappings.
Also applies to: 572-578, 1064-1070, 1194-1200, 1250-1256, 1957-1963, 2079-2085, 2165-2171
1616-1616:❓ Verification inconclusive
Verify impact of USDT symbol change.
The symbol change from 'USDT' to 'USD₮0' might affect existing integrations or UI components that rely on the symbol string.
🏁 Script executed:
#!/bin/bash # Search for direct usage of 'USDT' symbol in code rg -l "USDT" --type ts --type tsxLength of output: 61
Action: Confirm “USDT” References in TypeScript Files
The symbol was changed from “USDT” to “USD₮0”. Please verify that no component or integration still relies on the “USDT” string in the code. Note that the initial search with
--type tsxresulted in an error due to an unrecognized file type. To ensure complete coverage, please re-run the searches for both.tsand.tsxfiles by executing:#!/bin/bash # Corrected search for direct usage of 'USDT' in .ts and .tsx files echo "Searching in .ts files:" rg -l "USDT" --glob "*.ts" echo "Searching in .tsx files:" rg -l "USDT" --glob "*.tsx"Manually verify any occurrences found to confirm that the symbol change won’t affect existing integrations or UI components.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/synapse-interface/constants/tokens/bridgeable.ts (2)
46-50: Address the temporary logo usage for native tokens.The current implementation uses chain logos as token logos, which is marked as a TODO. This should be updated with the correct token-specific logos to maintain consistency with other token representations.
Would you like me to create an issue to track the logo updates for these native tokens?
1274-1275: Define the correct color for the BERA token.The color is currently set to a placeholder 'gray' value with a TODO comment. This should be updated to match BERA's brand color for consistency.
Would you like me to help determine and implement the correct brand color for the BERA token?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/synapse-interface/constants/tokens/bridgeable.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/synapse-interface/constants/tokens/bridgeable.ts (1)
1264-1343: LGTM! Well-structured token definitions.The new token definitions follow the established pattern consistently:
- Correct use of
zeroAddressfor native tokens- Consistent decimals (18) across all new tokens
- Proper
isNativeflag set to true- Consistent visibility and priority ranks (90 and 300 respectively)
- Appropriate swapableType and routeSymbol values
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/synapse-interface/constants/chains/master.tsx (1)
701-726: Address the TODO comment regarding the chain icon.The configuration is well-structured and consistent with other chains. However, there's a pending task to potentially use a different icon.
Would you like me to help track this task by opening a new issue for the icon update?
packages/synapse-interface/constants/tokens/bridgeable.ts (1)
1265-1280: Add color for BERA token.The TODO comment in the code indicates that the color needs to be corrected for the BERA token. Currently using a placeholder 'gray' color.
Consider choosing a distinctive color that aligns with the token's branding or the chain's theme.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/synapse-interface/constants/bridgeMap.ts(28 hunks)packages/synapse-interface/constants/chains/extraWagmiChains.ts(1 hunks)packages/synapse-interface/constants/chains/index.tsx(1 hunks)packages/synapse-interface/constants/chains/master.tsx(3 hunks)packages/synapse-interface/constants/chains/supportedChains.ts(2 hunks)packages/synapse-interface/constants/tokens/bridgeable.ts(3 hunks)packages/synapse-interface/scripts/data/providerOverrides.json(1 hunks)packages/synapse-interface/scripts/generateMaps.js(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/synapse-interface/scripts/data/providerOverrides.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/scripts/generateMaps.js
- packages/synapse-interface/constants/bridgeMap.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: SonarCloud
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (8)
packages/synapse-interface/constants/chains/supportedChains.ts (2)
30-30: LGTM!The import of
hyperEVMfollows the established pattern and is correctly sourced from the extraWagmiChains module.
60-60: LGTM!The addition of
hyperEVMto the supported chains array is consistent with the existing pattern.packages/synapse-interface/constants/chains/index.tsx (1)
47-47: LGTM!The
HYPEREVMchain ID is correctly defined and follows the alphabetical ordering convention.packages/synapse-interface/constants/chains/extraWagmiChains.ts (1)
148-173: LGTM!The
hyperEVMchain configuration follows the established pattern and includes all required properties. The multicall3 contract address matches the canonical address used across other chains.packages/synapse-interface/constants/chains/master.tsx (1)
657-658: LGTM!The RPC URL formatting has been condensed for consistency.
Also applies to: 683-684
packages/synapse-interface/constants/tokens/bridgeable.ts (3)
1265-1360: LGTM! The new token definitions follow the established pattern.The new token definitions for BERA, BNB, CANTO, CRO, GLMR, and HYPE follow the consistent pattern used throughout the file:
- All are native tokens (isNative: true)
- Use zeroAddress for their native chain
- Have consistent decimals (18)
- Follow the priorityRank pattern (300 for native assets)
- Include proper visibilityRank (90)
46-51:❓ Verification inconclusive
Verify the TODO comment and logo imports.
The TODO comment indicates that the logos for native tokens need to be corrected. The current implementation imports chain logos instead of token logos, which might not be the best representation for these tokens.
Run the following script to check if there are dedicated token logos available:
🏁 Script executed:
#!/bin/bash # Description: Check if dedicated token logos exist for the native tokens fd -t f "bera|bnb|canto|cro|glmr|hype" "assets/icons/"Length of output: 155
Manual verification required: Recheck dedicated token logos
The current implementation imports chain logos for native tokens, but the TODO indicates that we should eventually use dedicated token logos instead. The initial automated search in
assets/icons/failed because that directory isn’t valid—please manually verify if a dedicated folder for token logos exists or if they’re stored elsewhere.
- Ensure that the correct logo images for native tokens are available before replacing the current chain logos.
- If the dedicated token logos are located in another directory, update the import paths accordingly.
1265-1360:❓ Verification inconclusive
Verify token addresses across chains.
For native tokens that might be bridged to other chains, ensure that their addresses are correctly mapped.
Run the following script to check if these tokens exist on other chains:
🏁 Script executed:
#!/bin/bash # Description: Search for potential token addresses on other chains # Look for contract deployments or imports referencing these tokens rg -i "bera|bnb|canto|cro|glmr|hype" --type ts --type solLength of output: 87
ACTION REQUIRED: Confirm Token Address Mappings Across Chains
The token definitions in
packages/synapse-interface/constants/tokens/bridgeable.ts(lines 1265–1360) usezeroAddressfor each chain’s native token. Automated checks to locate these tokens in related TypeScript/Solidity files failed due to file type configuration issues. Please manually verify that the token addresses are correctly mapped for bridged native tokens and that there are no discrepancies in the chain integration.
- Verify that the native token entries (BERA, BNB, CANTO, CRO, GLMR, HYPE) have the appropriate address assignments.
- Confirm that any bridging logic or external contracts referencing these tokens properly recognize the
zeroAddressacross different chains.- Adjust the automated search command (e.g., by adding a file type for Solidity files) for future checks if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/synapse-interface/constants/tokens/bridgeable.ts (2)
5-5: Track the temporary usage of chain logos as token logos.The imports are currently using chain logos as token logos, which is marked by a TODO comment. While this works as a temporary solution, it should be tracked to ensure proper token-specific logos are added later.
Would you like me to create an issue to track the replacement of chain logos with proper token logos?
Also applies to: 47-51
46-46: Enhance the TODO comment with specific token information.The TODO comment could be more specific about which native tokens need logo corrections.
Consider updating the comment to list the specific tokens:
-// TODO: correct logo for these native tokens +// TODO: correct logo for native tokens: BERA, BNB, CANTO, CRO, GLMR, HYPEpackages/sdk-router/src/gaszip/gasZipModuleSet.ts (4)
34-43: Consider handling empty or invalid providers.If the
chainsarray is empty or improperly formatted, the module initialization might be incomplete or error-prone. A simple check or fallback will help ensure robust usage.
45-50: Return a clearer indicator when the module doesn't exist.Instead of
undefined, consider throwing an error or returning a more informative object if the requested chain is not supported. This can help diagnose issues in higher-level logic.
69-118: Handle errors from gas quote retrieval more gracefully.
getGasZipQuotecould fail due to network or API issues. Consider adding a try/catch to prevent unhandled promise rejections, and return a user-friendly fallback or empty route in case of exceptions.
21-173: Add targeted unit tests for the GasZipModuleSet class.This new class introduces important bridging logic. Comprehensive tests would confirm its correctness, especially for edge cases like unsupported chains and zero quotes.
Would you like help generating a set of unit tests for these methods?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sdk-router/src/gaszip/gasZipModuleSet.ts(1 hunks)packages/synapse-interface/constants/tokens/bridgeable.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (5)
packages/synapse-interface/constants/tokens/bridgeable.ts (1)
1265-1359: LGTM! Token definitions follow consistent patterns.The new token definitions for BERA, BNB, CANTO, CRO, GLMR, and HYPE are well-structured and consistent with the existing token patterns:
- Correct use of
zeroAddressfor native chain addresses- Consistent
priorityRank(300) andvisibilityRank(90) values- Proper
isNative: trueflag set- Appropriate swapableType and color values
packages/sdk-router/src/gaszip/gasZipModuleSet.ts (4)
19-19: Revisit the median Gas.zip bridging time.There was a previous request to adjust the constant from 30 to reflect near-instant bridging. If this value is indeed outdated, please update it accordingly.
-const MEDIAN_TIME_GAS_ZIP = 30 +const MEDIAN_TIME_GAS_ZIP = 0
62-64: Confirm whether zero gas drop is intended.If Gas.zip requires a minimal gas provision or has a future plan to drop gas for bridging, returning
Zeromay not be accurate. Confirm that consistently returning zero is correct for all scenarios.
120-136: Double-check zero fees assumption.If Gas.zip doesn’t impose any direct fee, returning a zero fee might be valid. Otherwise, ensure the logic remains in sync with any changes to cost structures.
152-164: Validate slippage settings.Although Gas.zip may not utilize slippage, confirm whether any route expansions or partial bridging could require a small tolerance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/synapse-interface/constants/bridgeMap.ts (1)
200-2154: Comprehensive update to integrate Gas.zip bridging module across multiple chains.The changes systematically add Gas.zip as an origin and destination option for native tokens across major blockchain networks. This implementation enables gas token bridging through the Gas.zip module for:
- ETH across multiple chains (Ethereum, Optimism, Base, Arbitrum, etc.)
- Native tokens on other chains (AVAX, FTM, BNB, CRO, BERA, etc.)
- New networks like Hyperlane (chain 999)
The consistent pattern of implementation suggests a well-structured approach to integrating this new bridging module.
Consider adding a brief documentation comment at the top of the file explaining the Gas.zip module's purpose and how it integrates with the existing bridge architecture, to help future developers understand this comprehensive update.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/synapse-interface/constants/bridgeMap.ts(19 hunks)packages/synapse-interface/constants/tokens/bridgeable.ts(3 hunks)packages/synapse-interface/scripts/utils/fetchGasZipData.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/scripts/utils/fetchGasZipData.js
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: changesets-integrity-checker
- GitHub Check: lint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (18)
packages/synapse-interface/constants/tokens/bridgeable.ts (2)
46-49: Address the TODO comment for logo correctionThere's a TODO comment indicating that the logos for these native tokens need to be corrected. Using chain logos as token logos can be confusing to users, as token logos typically have a different design style.
Is there a plan for creating proper token logos? Consider addressing this before finalizing the PR.
1263-1277: The new token definitions follow the established patternThe implementation of BERA, BNB, CRO, and HYPE tokens follows the same structure as other native tokens in the file, including correct priorityRank (300), proper isNative flag, and zeroAddress for native chain representation.
These token definitions align with the Gas.zip module requirements in the PR, enabling cross-chain bridging for these native assets.
Also applies to: 1279-1293, 1295-1309, 1311-1325
packages/synapse-interface/constants/bridgeMap.ts (16)
242-243: Gas.zip bridge module added to ETH token.'Gas.zip' has been added to both origin and destination arrays for ETH on Ethereum mainnet (chain '1'). This integrates the new Gas.zip bridging functionality for ETH.
444-445: Gas.zip bridge module added to ETH token on Optimism.Similar to the Ethereum mainnet changes, 'Gas.zip' has been added to both origin and destination arrays for ETH on Optimism (chain '10'). This is consistent with the cross-chain Gas.zip integration pattern.
467-473: New Gas.zip-only entry for CRO added on Cronos chain.A new entry has been added for CRO token on Cronos chain (ID: 25) with 'Gas.zip' as the only origin and destination, expanding the Gas.zip module functionality to the Cronos ecosystem.
572-578: New Gas.zip-only entry for BNB added on BSC.A new entry has been added for BNB token on BSC (chain '56') with 'Gas.zip' as the only origin and destination, enabling Gas.zip functionality for the native BNB token.
624-625: Gas.zip bridge module added to ETH token on Base.'Gas.zip' has been added to both origin and destination arrays for ETH on Base (chain '130'). This extends the Gas.zip module functionality to the Base ecosystem.
718-719: Gas.zip bridge module added to Polygon.'Gas.zip' has been added to both origin and destination arrays for the native Polygon token (POL) on chain '137', extending the Gas.zip functionality to Polygon.
824-825: Gas.zip bridge module added to Fantom.'Gas.zip' has been added to both origin and destination arrays for the native Fantom token (FTM) on chain '250', extending the Gas.zip functionality to Fantom.
942-949: New chain entry for Hyperlane (ID: 999) with HYPE token.A new chain entry for Hyperlane has been added with ID '999', containing a single token HYPE with 'Gas.zip' as the only origin and destination. This expands the Gas.zip module to the Hyperlane ecosystem.
1011-1012: Gas.zip bridge module added to Metis.'Gas.zip' has been added to both origin and destination arrays for the native Metis token on chain '1088', extending the Gas.zip functionality to Metis.
1776-1777: Gas.zip bridge module added to Avalanche.'Gas.zip' has been added to both origin and destination arrays for the native Avalanche token (AVAX) on chain '43114', extending the Gas.zip functionality to Avalanche.
1945-1951: New Gas.zip-only entry for BERA added on Berachain.A new entry has been added for BERA token on Berachain (chain '80094') with 'Gas.zip' as the only origin and destination, expanding the Gas.zip module functionality to the Berachain ecosystem.
1988-1989: Gas.zip bridge module added to ETH on multiple L2 chains.'Gas.zip' has been added to both origin and destination arrays for ETH on multiple L2 chains including Base (chain '81457'), Scroll (chain '534352'), and Linea (chain '59144'). This is consistent with the pattern of extending Gas.zip functionality across multiple chains for their native tokens.
Also applies to: 2024-2025, 1918-1919
518-524:✅ Verification successful
New entry for ETH on BSC with RFQ-only origin.
A new mapping for ETH has been added on BSC (chain '56') with 'RFQ.ETH' as the only origin and an empty destination array. This suggests one-way bridging functionality for ETH on BSC through RFQ.
🏁 Script executed:
#!/bin/bash # Check if there are any other tokens with empty destination arrays and how they're handled rg "destination: \[\]" --glob "*bridgeMap.ts"Length of output: 1844
Review Verified: Consistent Handling of One-Way Bridging
The new ETH entry on BSC (chain '56') in
packages/synapse-interface/constants/bridgeMap.ts(lines 518–524) correctly uses an empty destination array and lists'RFQ.ETH'as the sole origin. The recent search confirms that several mappings across the repository (including in the widget, rest-api, and synapse-constants packages) follow the same pattern, reinforcing that this one-way bridging configuration is intentional and consistent.
1604-1604:❓ Verification inconclusive
Token symbol changed from USDT to USD₮0.
The symbol for the token at address '0xFd086bC7CD5C481DCC9C85ebE478A1C0b69FCbb9' on Arbitrum (chain '42161') has been changed from 'USDT' to 'USD₮0'. This change might affect UI display and user recognition.
🏁 Script executed:
#!/bin/bash # Check for any UI components or code that explicitly references the USDT symbol rg "USDT['\"]|'USDT'|\"USDT\"" --glob "*.tsx" --glob "*.ts"Length of output: 10581
Action Required: Verify Consistency of Token Symbol Update
The token symbol for the token at address
0xFd086bC7CD5C481DCC9C85ebE478A1C0b69FCbb9on Arbitrum (chain42161) has been updated from'USDT'to'USD₮0'inpackages/synapse-interface/constants/bridgeMap.ts(line 1604). This change is intentional but might impact UI display and user recognition.Please note:
- A number of other files (e.g., in
packages/rest-api,packages/synapse-constants, andpackages/widget) still reference'USDT'. Confirm whether this update for the addressed token should be reflected universally or if it is meant to be isolated.- Verify that the UI components properly render the new symbol, and check for any inconsistencies that might confuse users who are expecting the traditional
'USDT'branding.
200-200:❓ Verification inconclusive
Order of destinations for USDC token changed.
The destination array for USDC on chain '1' has been reordered from
['CCTP.USDC', 'USDC', 'nUSD', 'RFQ.USDC']to['CCTP.USDC', 'RFQ.USDC', 'USDC', 'nUSD']. This may impact the priority or presentation of destination options in the interface.
🏁 Script executed:
#!/bin/bash # Check if this reordering affects any UI components that rely on this ordering rg -A 5 "destination.*USDC" --glob "*.tsx" --glob "*.ts"Length of output: 74881
USDC Destination Order Update – Please Verify UI Impact
The USDC destination array in
packages/synapse-interface/constants/bridgeMap.tshas been updated from
['CCTP.USDC', 'USDC', 'nUSD', 'RFQ.USDC']
to
['CCTP.USDC', 'RFQ.USDC', 'USDC', 'nUSD'].A search across the repository indicates that similar destination arrays are defined in several modules without any UI component explicitly relying on a fixed order. The related tests (e.g. in the SDK router files) appear to use these constants without enforcing a strict order in their assertions.
Please verify that:
- The new ordering correctly reflects the intended priority for destination presentation.
- No UI component or downstream logic depends on the original order.
- Any related tests are updated (if needed) to accommodate this change.
920-926:❓ Verification inconclusive
New entries added for Zkera Mainnet (chain 480).
Added WETH with RFQ.ETH origin and ETH with Gas.zip and RFQ.ETH origins on Zkera Mainnet (chain '480'). The ETH entry notably has 'Gas.zip' in origin but only in destination, indicating a potential asymmetry in bridging options.
Also applies to: 934-940
🏁 Script executed:
#!/bin/bash # Check if there are other tokens with asymmetric origin/destination Gas.zip configurations rg -A 4 -B 4 "origin:.*Gas\.zip.*\].*destination:.*\]" --glob "*bridgeMap.ts"Length of output: 79
Attention: Verify Bridging Asymmetry in Zkera Mainnet Token Configurations
It appears that for Zkera Mainnet (chain 480) new token entries were added. The WETH entry (lines 920–926) correctly uses
RFQ.ETHas its origin. However, the ETH entry (lines 934–940) includesGas.zipin its origin but not in its destination—creating a potential asymmetry in the bridging configuration. Since our automated search didn’t return any similar cases, please manually verify whether this discrepancy is intentional or if the ETH configuration should be adjusted (for example, by addingGas.zipto the destination or rebalancing the arrays for consistency).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (2)
123-145: Implement more robust error handling in checkGasZipTxStatusThe current error handling loses context from the original error, and there's limited type checking for the API response.
Consider these improvements:
const checkGasZipTxStatus = async ( txId: string ): Promise<GasZipStatus | undefined> => { try { const res = await fetch(`${GAS_ZIP_API_URL}/${txId}`, { method: 'GET' }) const data = await res.json() - if (!data.txs || !data.txs.length) { + if (!data || !Array.isArray(data.txs) || !data.txs.length) { return undefined } switch (data.txs[0].status) { case GasZipStatus.CONFIRMED: return GasZipStatus.CONFIRMED case GasZipStatus.CANCELLED: { // Check if there is a CONFIRMED tx in the list - this would be the refund tx return data.txs.find((tx) => tx.status === GasZipStatus.CONFIRMED) ? GasZipStatus.REFUNDED : GasZipStatus.CANCELLED } + default: + return undefined; } } catch (error) { - throw new Error(error) + console.error('Error checking Gas.zip transaction status:', error) + throw error // Re-throw the original error to preserve stack trace } }
127-131: Handle potential fetch errorsThe fetch operation doesn't check for HTTP error status codes.
Enhance the error handling for more robust operation:
- const res = await fetch(`${GAS_ZIP_API_URL}/${txId}`, { method: 'GET' }) - const data = await res.json() + const res = await fetch(`${GAS_ZIP_API_URL}/${txId}`, { method: 'GET' }) + if (!res.ok) { + throw new Error(`HTTP error! Status: ${res.status}`) + } + const data = await res.json()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/synapse-interface/components/_Transaction/_Transaction.tsx(1 hunks)packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: SonarCloud
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
packages/synapse-interface/components/_Transaction/_Transaction.tsx (1)
112-112:❓ Verification inconclusive
Bridge module name condition expanded to include Gas.zip
The condition has been updated to check if
bridgeModuleNameis either 'SynapseRFQ' or 'Gas.zip' when determining transaction refund status, which aligns with the addition of the new Gas.zip module.Consider if this approach will scale well as more bridge modules are added. Run the following to check for other potential bridge modules in the codebase:
🏁 Script executed:
#!/bin/bash # Find potential bridge module names in the codebase rg -i "bridgeModuleName.*=.*['\"]" --no-filename | sort | uniqLength of output: 799
Bridge Module Condition Update Approved – Consider a Scalable Approach
The condition in
packages/synapse-interface/components/_Transaction/_Transaction.tsx(line 112) has been updated to:['SynapseRFQ', 'Gas.zip'].includes(bridgeModuleName)This correctly adds
'Gas.zip'alongside'SynapseRFQ'for determining transaction refund eligibility. However, our codebase search uncovered additional bridge module names such as'SynapseBridge','SynapseCCTP', and even a check for'ALL'. As more modules might be introduced in the future, please review whether this hardcoded approach is sustainable or if a centralized, data-driven configuration would better address scaling concerns.packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (3)
20-28: Added Gas.zip status enum and constantsThe new enum and constants provide clear definitions for handling Gas.zip transaction statuses.
40-48: Added Gas.zip transaction status checkGood implementation of the Gas.zip status check based on the router address. This correctly handles the different flow for Gas.zip transactions compared to RFQ transactions.
76-76: Consider implementing the TODOThere's a TODO comment about moving the RFQ bridge contract logic to the sdk-router.
This refactoring would improve code organization and maintainability by centralizing related functionalities.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (3)
27-28: Consider using environment variables for configuration.Hardcoded URLs and addresses should ideally be moved to environment configuration to facilitate different environments (dev, staging, prod) and easier updates.
-const GAS_ZIP_API_URL = 'https://backend.gas.zip/v2/deposit' -const GAS_ZIP_DEPOSIT_ADDRESS = '0x391E7C679d29bD940d63be94AD22A25d25b5A604' +const GAS_ZIP_API_URL = process.env.NEXT_PUBLIC_GAS_ZIP_API_URL || 'https://backend.gas.zip/v2/deposit' +const GAS_ZIP_DEPOSIT_ADDRESS = process.env.NEXT_PUBLIC_GAS_ZIP_DEPOSIT_ADDRESS || '0x391E7C679d29bD940d63be94AD22A25d25b5A604'
77-77: Consider implementing TODO to move logic to sdk-router.As noted in the TODO comment, this Gas.zip integration logic should be moved to the sdk-router for better separation of concerns and code organization.
124-150: Add timeout and error handling for API call.The API call doesn't have a timeout which could lead to long waiting times if the service is slow to respond. Additionally, there's no handling for network errors or service unavailability.
const checkGasZipTxStatus = async ( txId: string ): Promise<GasZipStatus | undefined> => { try { - const res = await fetch(`${GAS_ZIP_API_URL}/${txId}`, { method: 'GET' }) + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 10000) // 10 second timeout + + try { + const res = await fetch(`${GAS_ZIP_API_URL}/${txId}`, { + method: 'GET', + signal: controller.signal + }) + clearTimeout(timeoutId) + + if (!res.ok) { + throw new Error(`API error: ${res.status} ${res.statusText}`) + } + + const data = await res.json() + // rest of the function... + } catch (fetchError) { + if (fetchError.name === 'AbortError') { + throw new Error('Gas.zip API request timed out') + } + throw fetchError + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/synapse-interface/components/_Transaction/_Transaction.tsx(3 hunks)packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/components/_Transaction/_Transaction.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: SonarCloud
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: changesets-integrity-checker
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (1)
20-25: Good addition of structured GasZipStatus enum.The enum provides clear semantics for different transaction states in Gas.zip. The descriptive comments help understand the business logic behind each status.
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts
Show resolved
Hide resolved
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (1)
131-131: 🛠️ Refactor suggestionImprove error handling in catch blocks
Both
getRFQBridgeContractandcheckRFQTxBridgeStatushave problematic error handling that loses the original stack trace.- throw new Error(error) + throw error instanceof Error ? error : new Error(String(error))Also applies to: 155-155
🧹 Nitpick comments (1)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (1)
20-63: Well-structured type definitions and constantsThe addition of the new types, interfaces, and constants for Gas.zip integration is well-organized. However, consider moving the hardcoded constants (
GAS_ZIP_API_URLandGAS_ZIP_DEPOSIT_ADDRESS) to a configuration file for better maintainability.-const GAS_ZIP_API_URL = 'https://backend.gas.zip/v2/deposit' -const GAS_ZIP_DEPOSIT_ADDRESS = '0x391E7C679d29bD940d63be94AD22A25d25b5A604' +import { GAS_ZIP_API_URL, GAS_ZIP_DEPOSIT_ADDRESS } from '@/constants/gaszip'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Deploy to Vercel (synapse-interface)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
packages/synapse-interface/components/_Transaction/helpers/useTxRefundStatus.ts (4)
76-84: Address null safety for txId parameterIf
txIdis undefined, the code will attempt to call the API with an invalid parameter. Add a null check before proceeding.if ( routerAddress.toLowerCase() === GAS_ZIP_DEPOSIT_ADDRESS.toLowerCase() ) { + if (!txId) { + console.error('Transaction ID is missing for Gas.zip status check') + return + } const status = await checkGasZipTxStatus(txId) if (status === GasZipStatus.REFUNDED) { setIsRefunded(true) } return }
159-182: Fix several issues in the checkGasZipTxStatus functionThe function has several issues that should be addressed:
- Improve error handling to preserve stack traces
- Add response status check
- Fix logic flow for clarity in status checking
- Add stronger typing for API response
const checkGasZipTxStatus = async (txId: string): Promise<GasZipStatus> => { try { const res = await fetch(`${GAS_ZIP_API_URL}/${txId}`, { method: 'GET' }) + if (!res.ok) { + throw new Error(`API error: ${res.status} ${res.statusText}`) + } const data = (await res.json()) as TransactionStatusData if (!data.txs || !data.txs.length) { return GasZipStatus.OTHER } + + // Check for CONFIRMED status first + if (data.txs[0].status === GasZipStatus.CONFIRMED) { + return GasZipStatus.CONFIRMED + } + if ( data.txs[0].status === GasZipStatus.CANCELLED || data.txs[0].cancelled ) { // Check if there is a refund tx in the list return data.txs.find((tx) => tx.refund) ? GasZipStatus.REFUNDED : GasZipStatus.CANCELLED } - if (data.txs[0].status === GasZipStatus.CONFIRMED) { - return GasZipStatus.CONFIRMED - } return GasZipStatus.OTHER } catch (error) { - throw new Error(error) + console.error('Failed to check Gas.zip transaction status:', error) + throw error instanceof Error ? error : new Error(String(error)) } }
112-112: Address the TODO commentConsider moving this logic to the SDK router as noted in the TODO comment. This would improve code organization and maintainability.
Is there a specific timeline or associated issue for this refactoring task?
66-66:❓ Verification inconclusive
Parameter type change needs implementation verification
You've changed
txIdfrom optional to required. Ensure all callers of this hook provide a non-nulltxId.
🏁 Script executed:
#!/bin/bash # Search for places where useTxRefundStatus is called without a txId rg "useTxRefundStatus\(\s*(?!\w)" --type typescriptLength of output: 307
Below is a new verification script that uses the PCRE2 flag to check for potential issue cases. In particular, it searches for calls where the hook might be invoked without any arguments (i.e. an empty parameter list) or where
null/undefinedis passed fortxId:#!/bin/bash echo "Checking for calls to useTxRefundStatus with an empty parameter list:" rg --pcre2 "useTxRefundStatus\\(\\s*\\)" --type typescript echo "Checking for calls where txId is explicitly null or undefined:" rg --pcre2 "useTxRefundStatus\\(\\s*(null|undefined)" --type typescriptPlease run the above script to verify that all callers of
useTxRefundStatusnow pass a valid (non-null) transaction ID. Once you’ve confirmed there are no calls with missing or invalidtxIdvalues, you can consider this change verified.
Transaction ID Parameter Verification Needed
ChangingtxIdfrom optional to required means all callers must now provide a non-null value. Please verify that:
- No call sites invoke
useTxRefundStatuswithout arguments (i.e. an empty parameter list).- No call passes
nullorundefinedas thetxId.You can use the provided scripts (with the PCRE2 flag) to search the codebase. Once verified, you can remove this note.
Description
Adds Gas.zip bridge module.
TODO:
Summary by CodeRabbit