-
Notifications
You must be signed in to change notification settings - Fork 559
[SDK] Always show pay modal even on non-supported UB chains #7092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Changes the behavior of buyWithCrypto to use the Universal Bridge service instead of the legacy API - Updates getQuote and getTransfer implementation to use Bridge.Buy and Bridge.Sell - Adds a new Transfer module to the Bridge namespace for token transfers - Removes the legacy API endpoints from definitions.ts 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Adds new Transfer module for direct token transfers - Implements prepare function for getting transfer quotes and transactions - Adds tests for the Transfer module 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Switch useSendTransaction and useSwapSupportedChains to use Bridge.routes endpoint for retrieving supported destinations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Changed import from Bridge to Transfer for better clarity and consistency. - Updated the usage of the Transfer module in the getBuyWithCryptoTransfer function.
…b.com/thirdweb-dev/js into greg/tool-4506-update-routes-endpoint
<!-- ## title your PR with this format: "[SDK/Dashboard/Portal] Feature/Fix: Concise title for the changes" If you did not copy the branch name from Linear, paste the issue tag here (format is TEAM-0000): ## Notes for the reviewer Anything important to call out? Be sure to also clarify these in your comments. ## How to test Unit tests, playground, etc. --> <!-- start pr-codex --> --- ## PR-Codex overview This PR introduces documentation and assets for integrating thirdweb tools into the Stylus contract development workflow, specifically for Rust. It includes setup instructions, publishing and deployment commands, and images to enhance the documentation. ### Detailed summary - Added new sections in `page.mdx` for Arbitrum Stylus. - Included prerequisites for Rust and Solidity installations. - Provided setup commands for creating a Stylus project. - Added commands for publishing and deploying Stylus contracts. - Included images (`stylus-publish-page.png`, `stylus-deploy-page.png`) to illustrate the process. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` <!-- end pr-codex -->
Signed-off-by: samina <[email protected]>
Signed-off-by: Maximilian Hubert <[email protected]>
Signed-off-by: samina <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
<!-- start pr-codex --> ## PR-Codex overview This PR focuses on updating the pricing plans and features for the embedded wallets in the dashboard application. It adjusts the plan requirements and modifies the features associated with different plans. ### Detailed summary - Changed `authRequiredPlan` from `"accelerate"` to `"growth"`. - Added `"Custom Wallet Branding & Auth"` to the features list. - Removed `"Custom Wallet Branding & Auth"` from the `TEAM_PLANS` features. - Increased `$400 Usage Credits` to `$500 Usage Credits` in the `TEAM_PLANS` features. > ✨ Ask PR-Codex anything about this PR by commenting with `/codex {your question}` <!-- end pr-codex -->
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx (1)
252-260
: 💡 Verification agent🧩 Analysis chain
Notify users when funds become sufficient
When
insufficientFunds
changes from true to false, the UI simply replaces the waiting badge with a continue button, but users don't receive a notification that their funds have arrived.Consider implementing a visual or audio notification when funds arrive:
+ const [prevInsufficientFunds, setPrevInsufficientFunds] = useState(insufficientFunds); + + useEffect(() => { + // Detect when funds become sufficient + if (prevInsufficientFunds && !insufficientFunds) { + // Show notification that funds have arrived + // This could use a toast library or a simple animation + const audio = new Audio("path-to-notification-sound.mp3"); + audio.play().catch(console.error); // Handle potential autoplay restrictions + } + setPrevInsufficientFunds(insufficientFunds); + }, [insufficientFunds, prevInsufficientFunds]); + {insufficientFunds ? ( <WaitingBadge> Waiting for funds on {transactionCostAndData.chainMetadata.name}... </WaitingBadge> ) : ( <Button variant="accent" onClick={props.onDone} fullWidth> Continue </Button> )}
🏁 Script executed:
#!/bin/bash # Check if there's any explicit notification when funds become sufficient rg "insufficientFunds.*(false|!insufficientFunds)" --glob "*.{tsx,ts}" -A 5 -B 5Length of output: 82
🏁 Script executed:
#!/bin/bash # Inspect insufficientFunds usage in DepositScreen.tsx rg -n "insufficientFunds" packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx -A5 -B5 # Check for existing useEffect hooks in DepositScreen.tsx rg -n "useEffect" packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx -nLength of output: 1813
Add a notification when funds become sufficient
It looks like there’s no code that alerts the user when
insufficientFunds
flips fromtrue
tofalse
, so the badge silently turns into the Continue button. Users may miss that their funds have arrived.File:
• packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx (around lines 252–260)Suggested change: add a
useEffect
to detect the transition and fire a toast or play a sound. For example:+ import { useState, useEffect } from "react"; + import { toast } from "your-toast-library"; // or use your existing notification hook export function DepositScreen(props: DepositScreenProps) { // … const insufficientFunds = transactionCostAndData.walletBalance.value < totalCost; + const [prevInsufficientFunds, setPrevInsufficientFunds] = useState(insufficientFunds); + useEffect(() => { + if (prevInsufficientFunds && !insufficientFunds) { + toast.success("Funds have arrived — you can continue now."); + const audio = new Audio("/path/to/notification-sound.mp3"); + audio.play().catch(console.error); + } + setPrevInsufficientFunds(insufficientFunds); + }, [insufficientFunds, prevInsufficientFunds]); return ( // … {insufficientFunds ? ( <WaitingBadge>Waiting for funds on {transactionCostAndData.chainMetadata.name}…</WaitingBadge> ) : ( <Button variant="accent" onClick={props.onDone} fullWidth> Continue </Button> )} // … ); }
- Use your existing toast/notification system for consistency.
- Verify the sound file path or make it configurable.
🧹 Nitpick comments (5)
packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx (4)
139-143
: Improve faucet URL parameterizationThe hardcoded URL might be better structured as a configuration parameter to allow for easier changes or customization in the future.
- const openFaucetLink = () => { - window.open( - `https://thirdweb.com/${props.tx.chain.id}?utm_source=ub_deposit`, - ); - }; + const openFaucetLink = () => { + const baseUrl = "https://thirdweb.com"; + const chainPath = `${props.tx.chain.id}`; + const utmParam = "utm_source=ub_deposit"; + window.open(`${baseUrl}/${chainPath}?${utmParam}`); + };
97-101
: Consider making refetch interval configurableThe 10-second interval for refreshing transaction cost and balance data is hardcoded. Consider making this a configurable prop to allow for adjustments based on different chain characteristics or usage scenarios.
- refetchIntervalMs: 10_000, + refetchIntervalMs: props.refetchIntervalMs || 10_000,And update the props interface:
export function DepositScreen(props: { onBack: (() => void) | undefined; connectLocale: ConnectLocale; client: ThirdwebClient; tx: PreparedTransaction; onDone: () => void; + refetchIntervalMs?: number; }) {
73-83
: Update JSDoc to include detailed description of the component's purposeThe JSDoc comment is minimal and doesn't fully explain the component's purpose and usage.
/** - * - * @internal + * A screen component for showing deposit instructions when a user has insufficient funds. + * Displays the wallet address with QR code, current balance, and required additional funds. + * Shows a waiting indicator until sufficient funds are detected, then enables continuation. + * For testnet chains, provides a button to access the faucet for obtaining test tokens. + * + * @internal */
139-143
: Consider handling potential window.open() security concernsThe
window.open()
call could be blocked by popup blockers or may not work in all contexts. Consider handling the case where the window fails to open.const openFaucetLink = () => { - window.open( - `https://thirdweb.com/${props.tx.chain.id}?utm_source=ub_deposit`, - ); + const newWindow = window.open( + `https://thirdweb.com/${props.tx.chain.id}?utm_source=ub_deposit`, + ); + + // Handle case where popup is blocked + if (!newWindow || newWindow.closed || typeof newWindow.closed === 'undefined') { + console.warn('Popup window was blocked. Please allow popups for this site.'); + // Could show a user-facing message here + } };packages/thirdweb/src/react/web/ui/TransactionButton/ExecutingScreen.tsx (1)
45-45
: Consider adding more specific error handlingThe current implementation captures the error as-is, but you could enhance this by providing more user-friendly error messages for common transaction failures.
- setTxError(e as Error); + // Format user-friendly error messages for common transaction failures + const formattedError = formatTransactionError(e as Error); + setTxError(formattedError); // Add this function somewhere in your utilities function formatTransactionError(error: Error): Error { const message = error.message || ''; // Check for common error patterns and provide clearer messages if (message.includes('user rejected')) { return new Error('Transaction was rejected in your wallet'); } if (message.includes('insufficient funds')) { return new Error('Your wallet does not have enough funds to complete this transaction'); } // Add more cases as needed return error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/playground-web/src/components/pay/transaction-button.tsx
(2 hunks)packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx
(1 hunks)packages/thirdweb/src/react/web/ui/TransactionButton/ExecutingScreen.tsx
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/playground-web/src/components/pay/transaction-button.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Unit Tests
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx (1)
152-165
: Ensure numerical precision is maintained when formatting token amountsYou've addressed a previous review comment about using
Number.parseFloat()
instead ofNumber()
, which is good for avoiding potential precision loss with large token amounts.packages/thirdweb/src/react/web/ui/TransactionButton/ExecutingScreen.tsx (3)
27-27
: Good addition of explicit transaction error stateAdding a dedicated state variable for transaction errors improves error handling by separating transaction-specific errors from the hook's internal errors.
35-35
: Proper error state reset before transactionResetting the error state before attempting a transaction prevents stale errors from persisting.
87-87
: Error message display improvementThe implementation now correctly uses the dedicated
txError
state, providing better separation of concerns.
PR-Codex overview
This PR focuses on enhancing the transaction handling in the
thirdweb
package by introducing a deposit modal for unsupported tokens, improving error handling, and refining UI components related to transactions.Detailed summary
ExecutingTxScreen
withtxError
state.TransactionModal
to includemodalMode
for distinguishing between buy and deposit actions.DepositScreen
component for handling deposits.chainMetadata
.Summary by CodeRabbit
New Features
Enhancements
Bug Fixes