Change waiting time for transactions #192
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (4)
142-142: Consider documenting the security implications of reduced confirmation blocksThe removal of explicit block confirmation count (from 2 to default) improves the demo experience by reducing waiting time. However, this change increases vulnerability to chain reorganizations.
Consider adding a comment above these transactions to document:
- The intentional reduction in confirmation blocks
- The security implications
- The context (demo environment vs production)
+ // Note: Using default confirmation blocks to improve demo UX. + // This reduces security against chain reorganizations but is acceptable for demonstration purposes. + // In production environments, consider waiting for multiple confirmations. await paymentTx.wait();Also applies to: 175-175
Line range hint
144-148: Add timeout and exponential backoff to balance checkingThe current implementation uses a fixed 1-second delay which could lead to:
- Infinite loops if the balance never updates
- Unnecessary network requests
- Poor handling of network issues
Consider implementing a timeout and exponential backoff:
- while (requestData.balance?.balance! < requestData.expectedAmount) { - requestData = await _request?.refresh(); - await new Promise((resolve) => setTimeout(resolve, 1000)); + let attempts = 0; + const maxAttempts = 10; + const baseDelay = 1000; + while (requestData.balance?.balance! < requestData.expectedAmount) { + if (attempts >= maxAttempts) { + throw new Error("Timeout waiting for balance update"); + } + requestData = await _request?.refresh(); + const delay = baseDelay * Math.pow(1.5, attempts); + await new Promise((resolve) => setTimeout(resolve, delay)); + attempts++; + }
Line range hint
89-94: Improve error type handling for unsupported networksThe current implementation uses string matching to detect unsupported network errors, which is fragile and maintenance-prone.
Consider using error types or error codes instead:
- if (String(err).includes("Unsupported payment")) { + if (err instanceof UnsupportedNetworkError || err?.code === 'UNSUPPORTED_NETWORK') { unsupportedNetwork = true; return; }Also, consider adding error logging for better debugging:
if (String(err).includes("Unsupported payment")) { unsupportedNetwork = true; + console.error('Unsupported network error:', { + network, + error: err, + requestId: request?.requestId + }); return; }
Line range hint
171-180: Enhance error handling specificity in approve functionThe current catch-all error handling might mask specific issues that require different handling or user messaging.
Consider handling specific error cases:
try { loading = true; if ( getPaymentNetworkExtension(requestData!)?.id === Types.Extension.PAYMENT_NETWORK_ID.ERC20_FEE_PROXY_CONTRACT ) { const approvalTx = await approveErc20(requestData!, signer); await approvalTx.wait(); approved = true; } loading = false; - } catch (err) { - console.error("Something went wrong while approving ERC20 : ", err); + } catch (err: any) { + const errorMessage = err?.code === 4001 + ? "Transaction rejected by user" + : err?.code === -32603 + ? "Internal error: insufficient funds" + : "Failed to approve ERC20"; + console.error(`${errorMessage}:`, err); + toast.error(errorMessage); loading = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:0-0
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte`, mapping payment network IDs to their respective approval functions in the `checkApproval` function is acceptable and can improve readability and maintainability.
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:236-247
Timestamp: 2024-11-18T04:03:25.560Z
Learning: In the `checkApproval` function of `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte` (TypeScript/Svelte code), remember that the `ETH_FEE_PROXY_CONTRACT` payment network ID does not require approval because Ether payments do not need ERC20 approvals.
sstefdev
changed the title
sstefdev
deleted the
154-reduce-the-block-confirmations-when-granting-approval-and-making-a-payment-with-the-invoice-dashboard
branch
Fixes: PR
Problem
When demonstrating Request Invoicing to potential app builders, the perceived slowness of the process poses a greater concern than the technical implications of chain reorganizations.
Changes
Removed arguments to
.wait()function ininvoice-viewSummary by CodeRabbit
New Features
Bug Fixes