feat: fail dropped jupiter transactions #553
Conversation
WalkthroughThe changes in this pull request focus on enhancing the transaction handling and status management within the swap functionality of the application. Key updates include the transition from using Changes
Possibly related PRs
Warning Rate limit exceeded@NickKelly1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
|
|
💼 Build Files |
![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 (17)
packages/swap/tests/zerox.test.ts (1)
28-31: LGTM! Consider documenting the CI skip reason.The placeholder test for CI environments is correctly implemented. However, it would be helpful to add a comment explaining why Zerox tests are skipped in CI (e.g., if it requires specific network conditions, API keys, or live endpoints).
if (process.env.CI) { + // Skip Zerox tests in CI as they require live network access // We need at-least one test otherwise vitest reports failure it("No ZeroX swap tests in CI", function () { expect(true).toBeTruthy(); }); return; }packages/swap/src/types/index.ts (1)
224-229: LGTM! Consider adding validation utilities.The new
StatusOptionTransactiontype is well-structured and documented. It provides the necessary fields to track transaction timing, which aligns with the PR objective of handling transaction timeouts.Consider adding a utility type or const for the timeout duration:
/** 3 minutes in milliseconds */ export const TRANSACTION_TIMEOUT_MS = 3 * 60 * 1000; export type TransactionTimeoutStatus = { isTimedOut: boolean; remainingMs: number; }; export const getTransactionTimeoutStatus = ( transaction: StatusOptionTransaction ): TransactionTimeoutStatus => ({ isTimedOut: Date.now() - transaction.sentAt > TRANSACTION_TIMEOUT_MS, remainingMs: Math.max(0, TRANSACTION_TIMEOUT_MS - (Date.now() - transaction.sentAt)) });packages/swap/src/providers/zerox/index.ts (1)
Add timeout handling for consistency with Jupiter provider
The review comment is valid. The Jupiter provider already implements the 3-minute timeout handling, while ZeroX and other EVM-based providers (OneInch, Paraswap) don't handle timeouts yet. Here's how to implement it consistently:
getStatus(options: StatusOptions): Promise<TransactionStatus> { const promises = options.transactions.map(({ hash, sentAt }) => { // Consider dropped if it's still null after 3 minutes if (Date.now() > sentAt + 3 * 60_000) { return Promise.resolve(null); } return this.web3eth.getTransactionReceipt(hash); }); return Promise.all(promises).then((receipts) => { for (const receipt of receipts) { if (!receipt || (receipt && !receipt.blockNumber)) { return TransactionStatus.pending; } if (receipt && !receipt.status) return TransactionStatus.failed; } return TransactionStatus.success; }); }
- The same timeout handling should also be added to
packages/swap/src/providers/oneInch/index.tsandpackages/swap/src/providers/paraswap/index.ts🔗 Analysis chain
Line range hint
275-286: Add timeout handling for consistency with other providers.While the parameter structure has been updated to support transaction timestamps, the
getStatusmethod doesn't implement the 3-minute timeout requirement mentioned in the PR objectives. This could lead to inconsistent behavior across different providers.Consider implementing timeout handling similar to other providers. Here's a suggested implementation:
getStatus(options: StatusOptions): Promise<TransactionStatus> { - const promises = options.transactions.map(({ hash }) => + const promises = options.transactions.map(({ hash, timestamp }) => { + // Check for timeout first + const now = Date.now(); + if (now - timestamp > 180000) { // 3 minutes in milliseconds + return Promise.resolve(null); // Simulate missing receipt for timeout + } + // If not timed out, check receipt this.web3eth.getTransactionReceipt(hash) - ); + }); return Promise.all(promises).then((receipts) => { // eslint-disable-next-line no-restricted-syntax for (const receipt of receipts) { if (!receipt || (receipt && !receipt.blockNumber)) { return TransactionStatus.pending; } if (receipt && !receipt.status) return TransactionStatus.failed; } return TransactionStatus.success; }); }Let's verify if other providers implement similar timeout handling:
Let me analyze the search results further to understand the timeout implementation across providers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timeout handling in other swap providers rg -A 10 "getStatus.*StatusOptions.*Promise.*TransactionStatus" packages/swap/src/providers/Length of output: 5952
Script:
#!/bin/bash # Search for timeout or timestamp handling in Jupiter provider rg -A 15 "sentAt.*hash.*options\.transactions" packages/swap/src/providers/jupiter/Length of output: 1410
packages/swap/src/providers/oneInch/index.ts (2)
Line range hint
298-309: Add timeout handling to align with PR objectives.The current implementation doesn't implement the 3-minute timeout requirement mentioned in the PR objectives. Transactions should be marked as failed if they haven't been mined within 3 minutes.
Here's a suggested implementation:
getStatus(options: StatusOptions): Promise<TransactionStatus> { + const THREE_MINUTES = 3 * 60 * 1000; // 3 minutes in milliseconds const promises = options.transactions.map(({ hash }) => - this.web3eth.getTransactionReceipt(hash) + Promise.all([ + this.web3eth.getTransactionReceipt(hash), + this.web3eth.getTransaction(hash) + ]).then(([receipt, tx]) => { + if (!receipt || (receipt && !receipt.blockNumber)) { + // Check if transaction is pending for more than 3 minutes + if (tx && Date.now() - Number(options.transactions.find(t => t.hash === hash)?.timestamp || 0) > THREE_MINUTES) { + return { status: false }; + } + return null; + } + return receipt; + }).catch(error => { + console.error(`Failed to fetch transaction status for ${hash}:`, error); + return null; + }) ); return Promise.all(promises).then((receipts) => { // eslint-disable-next-line no-restricted-syntax for (const receipt of receipts) { if (!receipt || (receipt && !receipt.blockNumber)) { return TransactionStatus.pending; } if (receipt && !receipt.status) return TransactionStatus.failed; } return TransactionStatus.success; }); }
Line range hint
298-309: Add type safety for the transactions array.The code assumes that the
transactionsarray contains objects with ahashproperty, but this isn't enforced through TypeScript types.Consider adding a type definition:
interface Transaction { hash: string; timestamp: number; } interface StatusOptions { transactions: Transaction[]; }packages/swap/src/providers/paraswap/index.ts (1)
Line range hint
353-364: Add timeout handling for pending transactionsThe current implementation doesn't implement the 3-minute timeout mentioned in the PR objectives. While this is a ParaSwap provider, we should maintain consistent behavior across all providers.
Consider implementing the timeout like this:
getStatus(options: StatusOptions): Promise<TransactionStatus> { + const THREE_MINUTES = 3 * 60 * 1000; + const now = Date.now(); + + // Check for timeout first + for (const tx of options.transactions) { + if (now - tx.timestamp > THREE_MINUTES) { + return Promise.resolve(TransactionStatus.failed); + } + } + const promises = options.transactions.map(({ hash }) => - this.web3eth.getTransactionReceipt(hash) + this.web3eth.getTransactionReceipt(hash).catch(error => { + console.error(`Failed to fetch receipt for ${hash}:`, error); + return null; + }) );packages/extension/src/ui/action/views/swap/views/swap-best-offer/index.vue (1)
Based on the search results, I can now provide a final response as there's enough context about the error handling in the codebase.
Consider using the existing error type system for better error handling
The codebase already has a structured error handling system that we should leverage:
- There's a
SwapErrorenum inpackages/extension/src/ui/action/views/swap/components/swap-error/types.tsfor swap-specific errors- For provider errors, there's an
ErrorCodesenum that includes specific error cases likeuserRejected (4001),unauthorized (4100), etc..catch(err => { console.error(err); isTXSendError.value = true; - TXSendErrorMessage.value = err.error ? err.error.message : err.message; + // Map provider error codes to user-friendly messages + let errorType = 'UNKNOWN_ERROR'; + if (err.code && Object.values(ErrorCodes).includes(err.code)) { + errorType = err.code === ErrorCodes.userRejected ? 'USER_REJECTED' : 'PROVIDER_ERROR'; + } + TXSendErrorMessage.value = errorType === 'USER_REJECTED' + ? 'Transaction was rejected' + : 'Failed to complete the swap'; trackSwapEvents(SwapEventType.swapFailed, { network: network.value!.name, fromToken: swapData.fromToken.name, toToken: swapData.toToken.name, swapProvider: pickedTrade.value.provider, - error: TXSendErrorMessage.value, + error: errorType, + errorCode: err.code, }); });🔗 Analysis chain
Line range hint
437-451: Enhance error handling and user feedbackThe current error handling implementation could be improved:
- Error messages might expose sensitive information directly to the UI.
- Generic error handling doesn't distinguish between different types of failures (network, timeout, user rejection, etc.).
Consider applying these changes:
.catch(err => { console.error(err); isTXSendError.value = true; - TXSendErrorMessage.value = err.error ? err.error.message : err.message; + // Sanitize and categorize errors + const errorType = err.code ? mapErrorCodeToType(err) : 'UNKNOWN_ERROR'; + TXSendErrorMessage.value = getUserFriendlyMessage(errorType); trackSwapEvents(SwapEventType.swapFailed, { network: network.value!.name, fromToken: swapData.fromToken.name, toToken: swapData.toToken.name, swapProvider: pickedTrade.value.provider, - error: TXSendErrorMessage.value, + error: errorType, + errorDetails: err.message, // Log full error details for debugging }); });Let's check for existing error handling patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for error handling patterns in swap-related code # Expected: Find error mapping implementations and user-friendly messages # Search for error handling in swap-related files rg -A 5 "catch\s*\(.*\)\s*{" "packages/extension/src/ui/action/views/swap" # Look for error type definitions ast-grep --pattern 'enum $_ { $$$ }'Length of output: 165
Script:
#!/bin/bash # Let's try a broader search for error handling patterns # Search for error-related code in the swap functionality rg -l "error|Error" "packages/extension/src/ui/action/views/swap" # Look for any error type definitions or mappings rg -A 5 "type.*Error|enum.*Error|interface.*Error" packages/extension # Check for existing error message handling or mapping functions rg -A 5 "message.*error|error.*message" "packages/extension/src/ui/action/views/swap" # Search for any error tracking or analytics rg -A 5 "trackSwapEvents.*error|SwapEventType" packages/extensionLength of output: 15988
packages/extension/src/ui/action/views/swap/libs/send-transactions.ts (4)
Line range hint
342-352: Consider extracting retry configuration.The backoff retry logic uses magic numbers. Consider extracting these into a configuration object for better maintainability.
- const backoff = [0, 500, 1_000, 2_000]; + const BLOCKHASH_UPDATE_CONFIG = { + retryDelays: [0, 500, 1_000, 2_000], + maxAttempts: 4, + }; + const backoff = BLOCKHASH_UPDATE_CONFIG.retryDelays;
241-249: Improve logging consistency.Consider using a structured logging approach instead of concatenating messages with template literals. This will make it easier to parse and analyze logs.
- console.warn( - `Failed to get latest blockhash after ${backoffi} attempts,` + - ` continuing with old block hash for legacy transaction...`, - ); + console.warn('Failed to update blockhash', { + attempts: backoffi, + transactionType: 'legacy', + action: 'continuing with old blockhash' + });Also applies to: 352-360
🧰 Tools
🪛 Biome
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.(lint/complexity/noUselessLabel)
Line range hint
236-243: Remove unnecessary labels.The
update_block_hashlabels are unnecessary and can be removed as flagged by static analysis.- update_block_hash: while (true) { + while (true) { if (backoffi >= backoff.length) { - break update_block_hash; + break; } // ... rest of the code try { const latest = await conn.getLatestBlockhash(); legacyTx.recentBlockhash = latest.blockhash; - break update_block_hash; + break; }Also applies to: 347-354
🧰 Tools
🪛 Biome
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.(lint/complexity/noUselessLabel)
78-78: Implementation of transaction timeout logic needed.While the changes add transaction timestamps, the actual logic to fail transactions after 3 minutes is not implemented. Consider adding a transaction monitor that:
- Tracks transaction status using the
sentAttimestamp- Marks transactions as failed if not mined within 3 minutes
- Handles cleanup of failed transactions
Would you like me to help design and implement the transaction timeout monitoring system?
packages/swap/src/providers/jupiter/index.ts (1)
590-590: Consider extracting the magic number to a named constantThe timeout value
3 * 60_000would be more maintainable as a named constant at the top of the file.+ /** Time after which a pending transaction is considered failed (3 minutes) */ + const TRANSACTION_TIMEOUT_MS = 3 * 60_000; - if (Date.now() > sentAt + 3 * 60_000) { + if (Date.now() > sentAt + TRANSACTION_TIMEOUT_MS) {packages/swap/src/providers/changelly/index.ts (3)
1074-1076: TODO comment aligns with PR objectives but needs implementation.The TODO comment correctly identifies the need to handle dropped Solana transactions after 3 minutes, which aligns with the PR objectives. However, the implementation is missing.
Would you like me to help implement the 3-minute timeout functionality? Here's a suggested approach:
async getStatus(options: StatusOptions): Promise<TransactionStatus> { - // TODO: If a Solana transaction hasn't been found after 3 minutes then consider dropping it - // I'm not sure how Rango's API handles Solana transactions being dropped... + // Check if this is a Solana transaction + if (options.network === NetworkType.Solana && options.transactions?.[0]?.timestamp) { + const currentTime = Date.now(); + const transactionTime = options.transactions[0].timestamp; + const threeMinutesInMs = 3 * 60 * 1000; + + // If transaction is older than 3 minutes, consider it failed + if (currentTime - transactionTime > threeMinutesInMs) { + return TransactionStatus.failed; + } + } const params: ChangellyApiGetStatusParams = {
Line range hint
142-207: Enhance error handling and type safety in changellyRequest method.The
changellyRequestmethod could benefit from improved error handling and type safety:
- Network errors should be handled explicitly
- Response type validation should be added
- Timeout handling could be more robust
Consider applying these improvements:
private async changellyRequest<T>( method: string, params: any, context?: { signal?: AbortSignal } ): Promise<ChangellyApiResponse<T>> { const signal = context?.signal; const aborter = new AbortController(); + const timeoutMs = 30_000; + + // Define error types + class ChangellyTimeoutError extends Error { + constructor() { + super(`Changelly API request timed out after ${timeoutMs}ms: ${method}`); + } + } + function onAbort() { aborter.abort(signal!.reason); } function onTimeout() { - aborter.abort( - new Error(`Changelly API request timed out ${BASE_URL} ${method}`) - ); + aborter.abort(new ChangellyTimeoutError()); } function cleanup() { clearTimeout(timeout); signal?.removeEventListener("abort", onAbort); } - const timeout = setTimeout(onTimeout, 30_000); + const timeout = setTimeout(onTimeout, timeoutMs); signal?.addEventListener("abort", onAbort); try { const response = await fetch(BASE_URL, { method: "POST", signal: aborter.signal, body: JSON.stringify({ id: uuidv4(), jsonrpc: "2.0", method, params, }), headers: [ ["Content-Type", "application/json"], ["Accept", "application/json"], ], }); + + // Handle non-200 responses + if (!response.ok) { + throw new Error(`Changelly API returned ${response.status}: ${response.statusText}`); + } + const json = (await response.json()) as ChangellyApiResponse<T>; + + // Validate response structure + if (!json || (typeof json !== 'object')) { + throw new Error('Invalid response format from Changelly API'); + } + return json; } catch (error) { + if (error instanceof ChangellyTimeoutError) { + throw error; + } + throw new Error(`Changelly API request failed: ${error.message}`); } finally { cleanup(); } }
1074-1076: Consider security implications of transaction timeout handling.While implementing the 3-minute timeout for Solana transactions, consider these security aspects:
- Ensure transaction status is properly persisted to prevent loss of transaction history
- Consider edge cases where network delays might cause false timeouts
- Implement proper logging for security audit trails
Consider implementing these security measures:
- Add detailed logging of transaction status changes
- Implement a recovery mechanism for edge cases
- Consider making the timeout duration configurable
- Add monitoring for timed-out transactions
packages/swap/src/providers/rango/index.ts (2)
Line range hint
1283-1293: Add error handling for Solana transaction status checksThe current implementation should handle potential network errors when checking Solana transaction statuses.
Apply this improvement:
- const sigStatuses = await ( - this.web3 as Connection - ).getSignatureStatuses(transactions.map(({ hash }) => hash)); + try { + const sigStatuses = await ( + this.web3 as Connection + ).getSignatureStatuses(transactions.map(({ hash }) => hash)); + // ... rest of the status checking logic + } catch (error) { + console.error('Failed to check Solana transaction status:', error); + return TransactionStatus.pending; + }
1300-1302: Optimize EVM transaction status checksThe current implementation could be more resilient to individual transaction failures.
Consider using
Promise.allSettledto handle individual transaction failures:- const receipts = await Promise.all( - transactions.map(({ hash }) => - (this.web3 as Web3Eth).getTransactionReceipt(hash) - ) - ); + const receiptResults = await Promise.allSettled( + transactions.map(({ hash }) => + (this.web3 as Web3Eth).getTransactionReceipt(hash) + ) + ); + const receipts = receiptResults.map(result => + result.status === 'fulfilled' ? result.value : null + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
packages/extension/src/ui/action/views/swap/index.vue(1 hunks)packages/extension/src/ui/action/views/swap/libs/send-transactions.ts(16 hunks)packages/extension/src/ui/action/views/swap/views/swap-best-offer/index.vue(2 hunks)packages/swap/src/providers/changelly/index.ts(1 hunks)packages/swap/src/providers/jupiter/index.ts(1 hunks)packages/swap/src/providers/oneInch/index.ts(1 hunks)packages/swap/src/providers/paraswap/index.ts(1 hunks)packages/swap/src/providers/rango/index.ts(4 hunks)packages/swap/src/providers/zerox/index.ts(1 hunks)packages/swap/src/types/index.ts(1 hunks)packages/swap/tests/changelly.test.ts(1 hunks)packages/swap/tests/zerox.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/extension/src/ui/action/views/swap/libs/send-transactions.ts
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.
(lint/complexity/noUselessLabel)
[error] 354-354: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.
(lint/complexity/noUselessLabel)
🔇 Additional comments (11)
packages/swap/tests/changelly.test.ts (1)
55-55: LGTM! Parameter structure updated correctly.
The change from transactionHashes to transactions aligns with the updated method signature.
packages/swap/src/types/index.ts (1)
233-233: Verify the interface change impact.
The change from transactionHashes to transactions is well-aligned with the new timing functionality. However, this is a breaking change that requires verification.
Let's verify the usage of this interface across the codebase:
✅ Verification successful
Let me try a different search strategy to verify the interface change impact.
Based on the search results, I can now provide a definitive response about the interface change impact:
Interface change is properly implemented across the codebase
The search results confirm that:
- All providers (Jupiter, Paraswap, 0x, OneInch, Changelly, Rango) have been updated to use the new
transactionsarray property - The implementations correctly handle the new array structure:
- Jupiter checks for single transaction:
options.transactions.length !== 1 - Paraswap/0x/OneInch map over transactions:
options.transactions.map(({ hash }) => - Rango/Changelly destructure and use transactions array:
const { requestId, transactions } = options
- Jupiter checks for single transaction:
- No remaining references to the old
transactionHashesproperty exist in the codebase
The breaking change has been properly propagated throughout all dependent code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old transactionHashes property
# and verify that all implementations properly handle the new transactions array.
echo "Checking for old transactionHashes usage..."
rg "transactionHashes" --type ts
echo "Verifying implementations of StatusOptions interface..."
ast-grep --pattern 'interface $_ implements StatusOptions {
$$$
}'
echo "Checking getStatus method implementations..."
ast-grep --pattern 'getStatus(options: StatusOptions) {
$$$
}'
Length of output: 487
Script:
#!/bin/bash
# Search for any usage of StatusOptions type/interface
echo "Checking StatusOptions usage..."
rg "StatusOptions" -A 3 --type ts
# Search for any property access with 'transactions' or 'transactionHashes'
echo -e "\nChecking property access patterns..."
ast-grep --pattern 'options.$_'
# Search for type definitions containing 'transactions' array
echo -e "\nChecking transactions array type definitions..."
ast-grep --pattern 'transactions: $_[]'
Length of output: 62398
packages/swap/src/providers/oneInch/index.ts (1)
Line range hint 298-309: Verify transaction receipt fetching across providers.
Let's verify if other swap providers (Jupiter, ParaSwap, Rango, ZeroX) have consistent transaction status checking implementations.
✅ Verification successful
Let me gather more specific information about the transaction status checking implementations in these files.
Transaction status checking is consistent across providers, no changes needed
The verification shows that OneInch, ParaSwap, and ZeroX providers have identical transaction receipt checking implementations:
- All check for missing receipts or missing block numbers to determine pending status
- All verify receipt.status to determine failed transactions
- All use the same pattern:
!receipt || (receipt && !receipt.blockNumber)for pending andreceipt && !receipt.statusfor failed
Rango has a slightly different implementation due to its unique API requirements, but follows the same validation principles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check transaction status implementations across providers
# Expected: Find status checking implementations that might need similar updates
echo "Searching for getStatus method implementations across providers..."
ast-grep --pattern 'getStatus(options: $_) {
$$$
}'
echo "\nSearching for transaction receipt checking patterns..."
rg -l 'getTransactionReceipt|blockNumber|receipt.status' packages/swap/src/providers/
Length of output: 632
Script:
#!/bin/bash
# Get detailed implementation of getStatus and transaction receipt checking in each provider
echo "OneInch implementation:"
rg -A 10 "getStatus" packages/swap/src/providers/oneInch/index.ts
echo "\nParaswap implementation:"
rg -A 10 "getStatus" packages/swap/src/providers/paraswap/index.ts
echo "\nRango implementation:"
rg -A 10 "getStatus" packages/swap/src/providers/rango/index.ts
echo "\nZeroX implementation:"
rg -A 10 "getStatus" packages/swap/src/providers/zerox/index.ts
# Also check for any transaction receipt validation patterns
echo "\nTransaction receipt validation patterns:"
rg "receipt.status|!receipt|receipt &&" packages/swap/src/providers/
Length of output: 4445
packages/swap/src/providers/paraswap/index.ts (1)
Line range hint 353-364: Verify parameter type changes across providers
The change from transactionHashes to transactions objects is a breaking change that needs to be consistently implemented.
Let's verify the parameter structure across other providers:
packages/extension/src/ui/action/views/swap/views/swap-best-offer/index.vue (1)
Line range hint 398-424: Critical: Implement timeout handling and add array bounds check
The current implementation has several issues that need to be addressed:
- Missing timeout handling: According to PR objectives, transactions should fail after 3 minutes.
- No bounds checking: Accessing
txs[0]without verifying array length could cause runtime errors. - Transaction timing: The status tracking doesn't include transaction timestamp for timeout calculations.
Consider applying these changes:
.then(txs => {
+ if (!txs.length) {
+ throw new Error('No transaction hash received');
+ }
+
pickedTrade.value.status!.options.transactions = txs;
const swapRaw: SwapRawInfo = {
fromToken: swapData.fromToken,
toToken: swapData.toToken,
status: pickedTrade.value.status!,
};
const swapActivity: Activity = {
from: network.value!.displayAddress(account.value!.address),
to: swapData.toAddress,
token: {
decimals: swapData.toToken.decimals,
icon: swapData.toToken.logoURI,
name: swapData.toToken.name,
symbol: swapData.toToken.symbol,
coingeckoID: swapData.toToken.cgId,
price: swapData.toToken.price
? swapData.toToken.price.toString()
: '0',
},
isIncoming: account.value!.address === swapData.toAddress,
network: network.value!.name,
- status: ActivityStatus.pending,
+ status: ActivityStatus.pending,
+ createdAt: Date.now(),
+ timeoutAt: Date.now() + 180000, // 3 minutes in milliseconds
timestamp: new Date().getTime(),
type: ActivityType.swap,
value: pickedTrade.value.toTokenAmount.toString(),
transactionHash: `${txs[0].hash}-swap`,
rawInfo: JSON.parse(JSON.stringify(swapRaw)),
};Let's verify if the activity state implementation supports timeout tracking:
#!/bin/bash
# Description: Check if ActivityState handles transaction timeouts
# Expected: Find implementation details of transaction timeout handling
# Search for timeout-related code in ActivityState
rg -A 5 "timeoutAt|timeout" "packages/extension/src/libs/activity-state"
# Search for status update mechanisms
ast-grep --pattern 'class ActivityState {
$$$
updateStatus($$$) {
$$$
}
$$$
}'packages/extension/src/ui/action/views/swap/libs/send-transactions.ts (4)
119-119: LGTM! Bitcoin transaction handling updated correctly.
The Bitcoin transaction handling has been properly updated to include the sentAt timestamp.
176-176: LGTM! Substrate transaction handling updated correctly.
The Substrate transaction handling has been properly updated to include the sentAt timestamp.
Line range hint 507-569: LGTM! EVM transaction handling updated correctly.
The EVM transaction handling has been properly updated to include the sentAt timestamp while maintaining the existing transaction broadcasting logic.
78-78: LGTM! Return type change enables transaction timing.
The new return type including sentAt timestamp aligns with the PR objective to fail transactions after 3 minutes.
Let's verify that all consumers handle the new return type correctly:
✅ Verification successful
Return type change is correctly handled throughout the codebase
The new return type { hash: string, sentAt: number }[] aligns perfectly with the StatusOptionTransaction type definition in packages/swap/src/types/index.ts, which already expects both the hash and sentAt fields. The consumer in swap-best-offer/index.vue assigns the transactions directly to pickedTrade.value.status!.options.transactions, which expects this exact type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where executeSwap result is used
rg -A 3 "executeSwap\("
Length of output: 457
Script:
#!/bin/bash
# Get more context around executeSwap usage
rg -B 5 -A 10 "executeSwap\("
# Also search for any handling of the returned hash
rg "const.*=.*executeSwap" -A 3
Length of output: 1793
Script:
#!/bin/bash
# Let's check the actual usage of the transaction result with more context
rg -B 10 -A 10 "pickedTrade\.value\.status.*options\.transactions = txs"
# Also check the type definition of the status options
ast-grep --pattern 'interface $_ {
$$$
transactions: $_;
$$$
}'
Length of output: 4834
Script:
#!/bin/bash
# Let's check the StatusOptionTransaction type definition
ast-grep --pattern 'type StatusOptionTransaction = $_'
# And check for any existing usage of sentAt in the status handling
rg "sentAt.*status" -A 3
Length of output: 454
packages/swap/src/providers/jupiter/index.ts (2)
577-582: LGTM: Parameter validation and transaction extraction
The validation logic correctly ensures exactly one transaction is provided and properly extracts the transaction hash and timestamp using the new structure.
588-594: LGTM: Transaction timeout handling implementation
The implementation correctly fulfills the PR objective by failing transactions after 3 minutes, with good documentation explaining the timeout choice (2 minutes block hash expiry + 1 minute leeway).
NickKelly1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (20)
packages/extension/src/types/activity.ts (1)
125-131: Consider adding JSDoc comments to document the union type.The formatting change improves readability, and the inclusion of
SOLRawInfoaligns with the Solana transaction handling requirements.Consider adding JSDoc comments to document when each type is used:
+/** + * Raw information specific to different blockchain transactions: + * - EthereumRawInfo: For Ethereum transactions + * - SubstrateRawInfo: For Substrate-based chain transactions + * - SubscanExtrinsicInfo: For Subscan API responses + * - BTCRawInfo: For Bitcoin transactions + * - SwapRawInfo: For cross-chain swap transactions + * - KadenaRawInfo: For Kadena transactions + * - SOLRawInfo: For Solana transactions, includes timestamp for timeout tracking + */ rawInfo: | EthereumRawInfo | SubstrateRawInfo | SubscanExtrinsicInfo | BTCRawInfo | SwapRawInfo | KadenaRawInfo | SOLRawInfo;packages/extension/src/providers/solana/libs/api.ts (3)
26-26: Consider adding timeout configuration initialization.Given that this PR implements transaction timeout handling, consider initializing timeout-related configurations (e.g., 3-minute threshold) in this method.
33-36: Document version limitation.The
maxSupportedTransactionVersion: 0parameter might need documentation explaining why version 0 is used and any implications for newer transaction versions.
44-49: Simplify boolean conversion.The status conversion can be simplified.
- status: tx.meta?.err ? false : true, + status: !tx.meta?.err,🧰 Tools
🪛 Biome
[error] 48-48: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/extension/src/libs/activity-state/index.ts (4)
Line range hint
64-79: Consider improving error handling and validationThe
updateActivitymethod could benefit from several improvements:
- Input validation for activity and options parameters
- Error handling for storage operations
- More functional approach to array manipulation
Consider this refactor:
async updateActivity( activity: Activity, options: ActivityOptions, ): Promise<void> { + if (!activity?.transactionHash) { + throw new Error('Invalid activity: missing transaction hash'); + } const activities = await this.getActivitiesById( this.getActivityId(options), ); - const clone = [...activities]; - activities.forEach((act, idx) => { - if (act.transactionHash === activity.transactionHash) - clone[idx] = activity; - }); + const clone = activities.map(act => + act.transactionHash === activity.transactionHash ? activity : act + ); await this.setActivitiesById(clone, this.getActivityId(options)); }
80-85: Add error handling for storage operationsThe storage operation could fail and should be handled appropriately.
Consider adding error handling:
async setCacheTime(options: ActivityOptions): Promise<void> { + if (!options?.address || !options?.network) { + throw new Error('Invalid options: missing required fields'); + } + try { await this.#storage.set(this.getActivityCacheId(options), { [STORAGE_KEY]: new Date().getTime(), }); + } catch (error) { + throw new Error(`Failed to set cache time: ${error.message}`); + } }
94-97: Consider adding filtering and paginationThe method could benefit from:
- Filtering out expired transactions
- Pagination for large datasets
- Error handling
Consider this enhancement:
async getAllActivities(options: ActivityOptions): Promise<Activity[]> { + if (!options?.address || !options?.network) { + throw new Error('Invalid options: missing required fields'); + } + try { - return this.getActivitiesById(this.getActivityId(options)); + const activities = await this.getActivitiesById(this.getActivityId(options)); + const currentTime = new Date().getTime(); + return activities.filter(a => + a.status !== ActivityStatus.pending || + a.timestamp > currentTime - MAX_PENDING_TIME + ); + } catch (error) { + throw new Error(`Failed to get activities: ${error.message}`); + } }
Line range hint
102-116: Enhance security and validation in storage operationsThe private methods need better input validation and error handling to prevent potential security issues.
Consider these enhancements:
private async setActivitiesById( activities: Activity[], id: string, ): Promise<void> { + if (!id?.trim()) { + throw new Error('Invalid storage ID'); + } + if (!Array.isArray(activities)) { + throw new Error('Invalid activities array'); + } + try { await this.#storage.set(id, { [STORAGE_KEY]: activities, }); + } catch (error) { + throw new Error(`Storage operation failed: ${error.message}`); + } } private async getActivitiesById(id: string): Promise<Activity[]> { + if (!id?.trim()) { + throw new Error('Invalid storage ID'); + } + try { const allStates: Record<string, Activity[]> = await this.#storage.get(id); if (!allStates || !allStates[STORAGE_KEY]) return []; return allStates[STORAGE_KEY]; + } catch (error) { + throw new Error(`Storage operation failed: ${error.message}`); + } }packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue (3)
40-43: Consider visually distinguishing dropped vs failed states.The class bindings for both transaction and swap sections correctly handle the new
droppedstatus. However, there's currently no visual distinction between failed and dropped states, which might confuse users about why their transaction didn't complete.Consider adding distinct visual styles for dropped transactions to help users understand the difference between a failed transaction and one that timed out.
Also applies to: 94-97
267-269: Address the TODO comment with specific color suggestions.The dropped status should have a distinct color to differentiate it from failed transactions. Consider using:
- Orange (#FFA500) to indicate timeout/dropped status
- Red (#FF0000) for failed transactions
Would you like me to provide a complete CSS implementation for these color states?
Line range hint
40-269: Overall implementation feedback.The changes successfully implement support for dropped transactions, aligning with the PR objectives. The code structure is clean and follows Vue.js best practices. However, consider these improvements:
- Add visual distinction between failed and dropped states
- Validate the 3-minute timeout duration
- Add tooltips or help text to explain why transactions are marked as dropped
These enhancements would improve the user experience by making it clearer why transactions are in specific states.
packages/extension/src/ui/action/views/swap/libs/send-transactions.ts (5)
119-119: Consider implementing timeout handling for Bitcoin transactionsWhile the timestamp is now tracked, there's no implementation of the 3-minute timeout handling for Bitcoin transactions. Consider extending the timeout feature to Bitcoin transactions for consistency across networks.
176-176: Consider implementing timeout handling for Substrate transactionsWhile the timestamp is now tracked, there's no implementation of the 3-minute timeout handling for Substrate transactions. Consider extending the timeout feature to Substrate transactions for consistency across networks.
Line range hint
235-264: Refactor duplicated block hash update logicThe block hash update logic is duplicated between legacy and versioned transactions. Consider extracting this into a shared helper function.
+ async function updateBlockHash(conn: Connection, backoff: number[], kind: string) { + let backoffi = 0; + while (true) { + if (backoffi >= backoff.length) { + console.warn( + `Failed to get latest blockhash after ${backoffi} attempts,` + + ` continuing with old block hash for ${kind} transaction...` + ); + return null; + } + const backoffMs = backoff[backoffi]; + if (backoffMs > 0) { + console.warn( + `Waiting ${backoffMs}ms before retrying latest block` + + ` hash for ${kind} transaction...` + ); + await new Promise(res => setTimeout(res, backoffMs)); + } + try { + return await conn.getLatestBlockhash(); + } catch (err) { + console.warn( + `Failed to get latest blockhash on attempt` + + ` ${backoffi + 1}: ${String(err)}` + ); + } + backoffi++; + } + }Also applies to: 346-375
🧰 Tools
🪛 Biome
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.(lint/complexity/noUselessLabel)
Line range hint
507-569: Consider implementing timeout handling for EVM transactionsWhile timestamps are now tracked, there's no implementation of the 3-minute timeout handling for EVM transactions. Consider extending the timeout feature to EVM transactions for consistency across networks.
Additionally, consider implementing a transaction monitoring mechanism that:
- Tracks transaction status
- Marks transactions as failed if not mined within 3 minutes
- Provides feedback to the user about transaction timeouts
Line range hint
78-569: Implement a unified transaction timeout handlerWhile the changes successfully add timestamp tracking across all networks, consider implementing a unified transaction timeout handler that:
- Accepts the network type and transaction details
- Monitors transaction status across different networks
- Implements the 3-minute timeout logic consistently
- Updates the activity state when transactions timeout
- Provides appropriate user feedback
This would centralize the timeout logic and ensure consistent behavior across all supported networks.
Would you like me to help design this unified timeout handler?
🧰 Tools
🪛 Biome
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.(lint/complexity/noUselessLabel)
packages/extension/src/ui/action/views/network-activity/index.vue (4)
207-207: Remove unnecessary debugconsole.logstatementThe
console.logat line 207 appears to be a leftover debug statement:console.log('[[ ??? doing something ??? ]]', info);Consider removing it to clean up the console output.
150-236: Refactor duplicated code inhandleActivityUpdatefor maintainabilityThe
handleActivityUpdatefunction contains repetitive code blocks for each provider when updating the activity status and callingactivityState.updateActivity. Consider refactoring to eliminate duplication and improve readability.One approach is to extract common logic into a helper function. Here's an example:
const updateActivityStatus = ( activity: Activity, status: ActivityStatus, rawInfo: any, timer: ReturnType<typeof setInterval>, ) => { activity.status = status; activity.rawInfo = rawInfo; activityState .updateActivity(activity, { address: activityAddress.value, network: props.network.name, }) .then(() => updateVisibleActivity(activity)); // Clear the interval as we've reached a terminal status clearInterval(timer); };Then, in each provider case, you can use this helper function:
// Example for Ethereum provider if (props.network.provider === ProviderName.ethereum) { if (!info) return; const evmInfo = info as EthereumRawInfo; const status = evmInfo.status ? ActivityStatus.success : ActivityStatus.failed; updateActivityStatus(activity, status, evmInfo, timer); }
308-309: Re-evaluate clearing timers inonMountedIn the
onMounted()hook, you are clearing intervals and resetting theactivityCheckTimersarray:activityCheckTimers.forEach(timer => clearInterval(timer)); activityCheckTimers.length = 0;Since the component is newly mounted, there shouldn't be any existing timers. This code may be unnecessary and could be removed unless there's a specific reason for it.
150-236: Ensure proper typing for function parametersIn the
handleActivityUpdatefunction, thetimerparameter is typed asany. For better type safety, specify the parameter type explicitly.Update the function signature to:
const handleActivityUpdate = ( activity: Activity, info: any, timer: ReturnType<typeof setInterval>, ) => { ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
packages/extension/src/libs/activity-state/index.ts(3 hunks)packages/extension/src/providers/solana/libs/api.ts(1 hunks)packages/extension/src/types/activity.ts(2 hunks)packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue(4 hunks)packages/extension/src/ui/action/views/network-activity/index.vue(4 hunks)packages/extension/src/ui/action/views/swap/libs/send-transactions.ts(17 hunks)packages/swap/src/providers/jupiter/index.ts(1 hunks)packages/swap/src/types/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swap/src/providers/jupiter/index.ts
- packages/swap/src/types/index.ts
🧰 Additional context used
🪛 Biome
packages/extension/src/providers/solana/libs/api.ts
[error] 48-48: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/extension/src/ui/action/views/swap/libs/send-transactions.ts
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.
(lint/complexity/noUselessLabel)
[error] 354-354: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.
(lint/complexity/noUselessLabel)
🔇 Additional comments (7)
packages/extension/src/types/activity.ts (1)
98-98: LGTM! Verify all enum consumers are updated.
The new dropped status aligns with the PR objective to handle failed Jupiter transactions. The naming is clear and consistent with existing patterns.
Let's verify all consumers of this enum are updated:
✅ Verification successful
Let's gather more specific information about how ActivityStatus is used in these files to ensure the new 'dropped' status is properly handled.
The new dropped status is properly integrated across the codebase
Based on the search results, the new dropped status is already being handled appropriately in all relevant places:
- UI components properly handle the status in conditional rendering (network-activity-transaction.vue)
- Status updates include the new
droppedstate in activity tracking (network-activity/index.vue) - All activity handlers across different providers (ethereum, bitcoin, kadena, etc.) maintain consistent status handling
- The status is properly used in transaction verification components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files using ActivityStatus to ensure they handle the new 'dropped' status
# Test: Search for ActivityStatus usage. Expect: All consumers properly handle the 'dropped' case
# Find direct enum usage
rg -l "ActivityStatus\."
# Find switch/case statements that might need updating
ast-grep --pattern 'switch ($value) {
$$$
case $_: {
$$$
}
$$$
}'
Length of output: 2215
Script:
#!/bin/bash
# Search for switch statements specifically handling ActivityStatus
rg -U "switch.*ActivityStatus.*\{.*\}" -A 10
# Search for direct string comparisons with ActivityStatus
rg "ActivityStatus\.[a-zA-Z]+" -A 1
# Look for any conditional checks that might need updating
rg "if.*ActivityStatus\." -A 2
Length of output: 16795
packages/extension/src/providers/solana/libs/api.ts (2)
28-31: LGTM! Clear documentation of null cases.
The JSDoc comment clearly documents when null is returned, which is crucial for handling dropped transactions.
32-52: Verify consistent timeout handling across providers.
Let's ensure other Solana transaction providers (Rango, Changelly) implement consistent timeout handling.
🧰 Tools
🪛 Biome
[error] 48-48: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/extension/src/libs/activity-state/index.ts (1)
86-93: 🛠️ Refactor suggestion
Verify cache time implementation for failed transactions
This method is crucial for the PR's objective of failing transactions after 3 minutes. However, it needs better error handling and validation.
Let's verify the implementation aligns with the PR objectives:
Consider this enhancement:
async getCacheTime(options: ActivityOptions): Promise<number> {
+ if (!options?.address || !options?.network) {
+ throw new Error('Invalid options: missing required fields');
+ }
+ try {
const cacheTime: Record<string, number> = await this.#storage.get(
this.getActivityCacheId(options),
);
if (!cacheTime || !cacheTime[STORAGE_KEY]) return 0;
return cacheTime[STORAGE_KEY];
+ } catch (error) {
+ throw new Error(`Failed to get cache time: ${error.message}`);
+ }
}packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue (1)
195-197: Verify transaction timeout duration.
While the status handling for dropped transactions is implemented, there's no validation to ensure that transactions are marked as dropped after the specified 3-minute timeout mentioned in the PR objectives.
Consider adding a check to validate the timeout duration:
} else if (props.activity.status === ActivityStatus.dropped) {
+ const timeoutDuration = 3 * 60 * 1000; // 3 minutes in milliseconds
+ const currentTime = Date.now();
+ const transactionAge = currentTime - props.activity.timestamp;
+ if (transactionAge < timeoutDuration) {
+ console.warn('Transaction marked as dropped before 3-minute timeout');
+ }
status.value = 'Dropped';packages/extension/src/ui/action/views/swap/libs/send-transactions.ts (1)
78-78: LGTM: Return type enhancement supports transaction timing tracking
The modification to include sentAt timestamp is essential for implementing the 3-minute transaction timeout feature.
packages/extension/src/ui/action/views/network-activity/index.vue (1)
286-291: Verify necessity of monitoring activities with status dropped
In the setActivities function, you're initiating checkActivity for activities with status pending or dropped:
if (
(act.status === ActivityStatus.pending ||
act.status === ActivityStatus.dropped) &&
act.type === ActivityType.transaction
) {
checkActivity(act);
}Consider whether it's necessary to continue monitoring activities already marked as dropped, as they may have reached a terminal status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/swap/src/providers/jupiter/index.ts (1)
588-593: Add validation for sentAt timestampWhile the 3-minute timeout logic is sound, consider adding validation for the
sentAttimestamp to protect against:
- Invalid future timestamps
- Very old timestamps
- Clock skew between nodes
if (txResponse == null) { + // Validate sentAt timestamp + const now = Date.now(); + if (sentAt > now) { + throw new Error('Invalid sentAt timestamp: cannot be in the future'); + } + if (sentAt < now - 24 * 60 * 60 * 1000) { + throw new Error('Invalid sentAt timestamp: too old (max 24h)'); + } // Consider dropped (/failed) if it's still null after 3 minutes if (Date.now() > sentAt + 3 * 60_000) { return TransactionStatus.dropped; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
packages/swap/src/providers/changelly/index.ts(1 hunks)packages/swap/src/providers/jupiter/index.ts(1 hunks)packages/swap/src/providers/oneInch/index.ts(1 hunks)packages/swap/src/providers/paraswap/index.ts(1 hunks)packages/swap/src/providers/rango/index.ts(4 hunks)packages/swap/src/providers/zerox/index.ts(1 hunks)packages/swap/src/types/index.ts(2 hunks)packages/swap/tests/changelly.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/swap/src/providers/changelly/index.ts
- packages/swap/src/providers/oneInch/index.ts
- packages/swap/src/providers/paraswap/index.ts
- packages/swap/src/providers/rango/index.ts
- packages/swap/src/providers/zerox/index.ts
- packages/swap/src/types/index.ts
- packages/swap/tests/changelly.test.ts
🔇 Additional comments (3)
packages/swap/src/providers/jupiter/index.ts (3)
577-581: LGTM: Proper validation of transaction count
The check ensures exactly one transaction is provided, which is correct as Jupiter only supports single transactions.
582-582: LGTM: Clean destructuring of transaction data
Properly extracts both the timestamp and transaction hash needed for status checking.
583-586: LGTM: Proper transaction retrieval configuration
Correctly retrieves transaction details with appropriate version configuration for Solana compatibility.
If a Solana Jupiter swap transaction fails to be mined within 3 minutes then consider it dropped
Sets up ability to add the same functionality to Solana transactions from Rango and Changelly too.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style
Tests