merge main to develop #4856
merge main to develop #4856
Conversation
Fix/usdt donation on mainnet
HOTFIX: Updating wagmi and viem
HOTFIX: Update SelectTokenModal type issue
HOTFIX Moving from public to private RPC for mainnet viem client
removed stake together
added footer links to qacc
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Disable USDC on recurring donation
WalkthroughThis pull request introduces new localization entries for the Q/acc feature across multiple language files ( Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (2)next.config.js (2)
The implementation follows the established pattern and uses appropriate settings for a marketing asset redirect.
Please ensure that the PDF file exists at the specified location to prevent 404 errors. ✅ Verification successfulPDF asset exists at the specified location The verification confirms that the PDF file 'Giveth-Co-Marketing-Slides.pdf' exists at 'public/assets/Giveth-Co-Marketing-Slides.pdf', which matches the destination path in the redirect configuration. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the existence of the marketing PDF file
# Check if the PDF file exists in the public/assets directory
fd -t f "Giveth-Co-Marketing-Slides.pdf" public/assets/
# Also check if it's referenced in any documentation or README files
rg -i "Giveth-Co-Marketing-Slides.pdf" "README.md"
Length of output: 149 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: 3
🧹 Outside diff range and nitpick comments (20)
src/wagmiConfigs.ts (1)
41-43: Address TODO: Implement proper transport managementThe comment indicates this is a temporary solution. Consider implementing a more robust transport management system that:
- Handles all chains consistently
- Provides fallback mechanisms
- Implements proper error handling
Would you like assistance in designing a more comprehensive transport management solution?
src/lib/transaction.ts (1)
60-60: Type assertion looks good, consider adding runtime validationThe explicit type assertion to
0x${string}improves type safety for thegetTransactioncall. However, since type assertions bypass TypeScript's type checking at runtime, consider adding a validation helper.Consider adding a validation helper to ensure the hash format at runtime:
const isValidEthereumHash = (hash: string): hash is `0x${string}` => { return /^0x[0-9a-fA-F]{64}$/.test(hash); }; // Usage in the function: if (!isValidEthereumHash(txHash)) { throw new Error('Invalid Ethereum transaction hash format'); } hash: txHash, // No type assertion needed if validation passessrc/lib/constants/links.ts (1)
59-60: Consider environment-based URL configuration.While the current implementation works, consider moving service URLs to environment configuration for better flexibility across different environments (staging, development, etc.).
This would allow:
- Easier environment management
- Consistent URL handling across services
- Simplified deployment to different environments
Example approach:
const BASE_QACC_URL = process.env.NEXT_PUBLIC_QACC_URL || 'https://qacc.giveth.io/'; const links = { // ... QACC: BASE_QACC_URL, QACC_NEWS: `${BASE_QACC_URL}news`, // ... };src/hooks/useCreateEvmDonation.tsx (5)
Line range hint
33-57: Complete the migration of onError handlerThe commented out
onErrorhandler contains important logic for handling multi-sig transactions. This could lead to missing error handling for Safe transactions.Would you like me to help implement the migrated version of the
onErrorhandler? This would ensure proper handling of multi-sig transaction errors.
Line range hint
59-143: Improve error handling in handleSaveDonation
- The catch block uses
anytype which masks specific error types.- The backup API call uses potentially undefined variables (
transaction?.hash,transaction?.nonce,transaction?.from).Consider these improvements:
-} catch (e: any) { +} catch (e: unknown) { + const error = e as Error; await postRequest('/api/donation-backup', true, { chainId: chainId || token.networkId, - txHash: transaction?.hash, + txHash: transaction?.hash ?? txHash, amount: amount, token, projectId, anonymous, - nonce: transaction?.nonce, + nonce: transaction?.nonce ?? undefined, chainvineReferred, - walletAddress: transaction?.from, + walletAddress: transaction?.from ?? undefined, symbol: token.symbol, - error: e.message, + error: error.message, useDonationBox, relevantDonationTxHash, });
Line range hint
145-167: Enhance error handling structureThe current error handling could be more robust with better error discrimination and structured error reporting.
Consider enhancing the error handling:
const handleError = ( - error: any, + error: Error & { + replacement?: { hash: string }; + transactionHash?: string; + }, donationId: number, setFailedModalType: (type: EDonationFailedType) => void, ) => { console.error('name', error.name); const localTxHash = error.replacement?.hash || error.transactionHash; setTxHash(localTxHash); + const errorDetails = { + name: error.name, + message: error.message, + txHash: localTxHash, + donationId, + }; if (error.name === 'TransactionExecutionError') { setFailedModalType(EDonationFailedType.FAILED); } else { console.error('Rejected1', error); setFailedModalType(EDonationFailedType.REJECTED); } setDonationSaved(false); updateDonation(donationId, EDonationStatus.FAILED); - captureException(error, { tags: { section: 'confirmDonation' } }); + captureException(error, { + tags: { section: 'confirmDonation' }, + extra: errorDetails, + }); };
Line range hint
169-249: Fix potential memory leak and improve error handling
- The Promise resolution logic could lead to memory leaks if the component unmounts before resolution
- Using donationId as 0 in error handling might be incorrect for failed transactions
Consider these improvements:
+ const [promiseResolver, setPromiseResolver] = useState<(() => void) | null>(null); + useEffect(() => { + return () => { + if (promiseResolver) { + promiseResolver(); + setPromiseResolver(null); + } + }; + }, [promiseResolver]); await new Promise(resolve => { if (status === 'success') { - setResolveState(null); + resolve(); } else { - setResolveState(() => resolve); + setPromiseResolver(() => resolve); } }); - handleError(error, 0, setFailedModalType); + handleError(error, draftDonationData?.createDraftDonation ?? 0, setFailedModalType);
Line range hint
251-270: Fix missing effect dependenciesThe effect hook is missing dependencies that it uses within its callback.
Add missing dependencies:
useEffect(() => { if (status === 'success') { updateDonation(donationId, EDonationStatus.VERIFIED); setDonationMinted(true); if (resolveState) { resolveState(); setResolveState(null); } } const comingFromSafe = isSafeEnv && txHash; if (status === 'error' && !comingFromSafe) { updateDonation(donationId, EDonationStatus.FAILED); setDonationSaved(false); createDonationProps?.setFailedModalType(EDonationFailedType.FAILED); } - }, [status, donationId]); + }, [status, donationId, resolveState, isSafeEnv, txHash, createDonationProps]);src/components/views/donate/OneTime/SelectTokenModal/SelectTokenModal.tsx (5)
151-157: Simplify token filtering logic by removing unnecessary variable.The introduction of
sQueryadds unnecessary complexity. ThesearchQueryvariable can be used directly in the filtering logic.Consider this simpler implementation:
- const sQuery: string = searchQuery; const filtered = tokens.filter( token => token.symbol .toLowerCase() - .includes(sQuery.toLowerCase()) || - token.name.toLowerCase().includes(sQuery.toLowerCase()), + .includes(searchQuery.toLowerCase()) || + token.name.toLowerCase().includes(searchQuery.toLowerCase()), );
Line range hint
183-191: Improve error handling for balance fetching.The error from
fetchBalancesis only logged to console. Users should be notified when balance fetching fails, similar to how token data fetching errors are handled.Consider adding error notification:
} catch (error) { console.error('error on fetchTokenBalances', { error }); + setBalanceIsLoading(false); + showToastError('Failed to fetch token balances'); }
Line range hint
164-191: Optimize balance fetching performance.The balance fetching logic currently runs on every filter change due to
filteredTokensdependency. Consider caching the balances and only updating the filtered view.Consider this approach:
- Fetch balances only for
tokens(notfilteredTokens)- Store full balance map
- Filter the displayed tokens using the cached balances
// Add these state variables const [tokenBalanceMap, setTokenBalanceMap] = useState<Record<string, bigint>>({}); const [isInitialBalanceFetch, setIsInitialBalanceFetch] = useState(true); // Modify the fetch logic useEffect(() => { const fetchBalances = async () => { if (!isConnected || !tokens || !isInitialBalanceFetch) return; try { setBalanceIsLoading(true); const balances = isOnEVM ? await fetchEVMTokenBalances(tokens, walletAddress) : await Promise.all(/* ... */); const balanceMap = balances.reduce((acc, {token, balance}) => ({ ...acc, [token.address]: balance }), {}); setTokenBalanceMap(balanceMap); setIsInitialBalanceFetch(false); setBalanceIsLoading(false); } catch (error) { console.error('error on fetchTokenBalances', { error }); setBalanceIsLoading(false); showToastError('Failed to fetch token balances'); } }; fetchBalances(); }, [tokens, isConnected, isOnEVM, walletAddress, isInitialBalanceFetch]); // Use the balance map when displaying tokens const sortedTokens = filteredTokens.map(token => ({ token, balance: tokenBalanceMap[token.address] })).sort(/* ... */);
Line range hint
62-108: Add warning for custom token support.When users input custom token addresses, they should be warned about potential risks of interacting with unknown tokens.
Consider adding a warning message when a custom token is detected:
if (existingToken) { setCustomToken(undefined); setFilteredTokens([existingToken]); } else if (isOnEVM && acceptCustomToken) { + // Show warning for custom tokens + showToastError( + 'Warning: Custom tokens may be unsafe. Please verify the token contract carefully.', + { type: 'warning', duration: 6000 } + ); const initialToken = { address: searchQuery, decimals: 18, name: shortenAddress(searchQuery), symbol: shortenAddress(searchQuery),
Line range hint
256-267: Enhance input field accessibility.The search input field lacks proper accessibility attributes which are essential for screen readers and keyboard navigation.
Add ARIA attributes and labels:
const Input = styled.input` + /* existing styles */ padding: 8px 16px; padding-right: 48px; width: 100%; border: none; outline: none; font-size: 16px; background: transparent; line-height: 24px; `; + +// Usage in JSX: +<Input + placeholder='Search name or paste an address' + value={searchQuery} + onChange={e => setSearchQuery(e.target.value)} + aria-label="Search tokens" + role="searchbox" +/>src/hooks/usePassport.ts (3)
238-240: LGTM! Consider adding type validationThe type assertion for the Ethereum address improves type safety. However, consider adding runtime validation to ensure the address format is correct before the assertion.
Consider adding validation before the assertion:
+ if (!address.match(/^0x[a-fA-F0-9]{40}$/)) { + throw new Error('Invalid Ethereum address format'); + } const userAddressScore = await scoreUserAddress( address as `0x${string}`, );
Line range hint
391-391: Remove debugging console.log statementsThere are multiple console.log statements that appear to be debugging code. These should be removed as they:
- Expose sensitive user data and addresses
- Add noise to the browser console
- May impact performance in production
Remove all console.log statements or replace them with proper logging if needed:
- console.log('******0', address, isUserFullFilled, user); - console.log('******1', address, isUserFullFilled, user); - console.log('******2', address, isUserFullFilled, user); - console.log('******3', address, isUserFullFilled, user); - console.log('******4', address, isUserFullFilled, user); - console.log('******5', address, isUserFullFilled, user); - console.log('******6', address, isUserFullFilled, user); - console.log('******7', address, isUserFullFilled, user);Also applies to: 393-393, 396-396, 398-398, 400-400, 402-402, 404-404, 406-406, 408-408
Line range hint
242-245: Enhance error handling with more contextThe current error handling could be improved to provide more context about what went wrong during the score fetching process.
Consider enhancing the error handling:
} catch (error) { - console.error('Failed to fetch user address score:', error); + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + console.error(`Failed to fetch MBD score for address ${address}:`, { + error: errorMessage, + context: { address, activeQFRound: activeQFRound?.id } + }); updateState(user!); }src/lib/helpers.ts (3)
379-381: Improve the comment about the viem package issueThe current comment about the viem package issue is not very clear. Consider adding more context or a reference to the issue.
-// 'viem' ABI contract for USDT donation fails on mainnet -// so we use the USDT mainnet ABI instead and put inside usdtMainnetABI.json file -// update for 'viem' package to fix this issue doesn't work +// Using custom USDT mainnet ABI due to incompatibility with viem's default ERC20 ABI +// See: [Add link to the viem issue here] +// TODO: Remove this workaround once the viem package issue is resolved
391-400: Improve type safety for decimals handlingThe current implementation could be improved to handle decimals more safely:
- Add type assertion for the readContract result
- Add validation for the decimals value
-let decimals = await readContract(wagmiConfig, { +const decimalsResult = await readContract(wagmiConfig, { ...baseProps, functionName: 'decimals', -}); +}) as bigint | number; -if (typeof decimals === 'bigint') { - decimals = Number(decimals.toString()); +const decimals = typeof decimalsResult === 'bigint' + ? Number(decimalsResult.toString()) + : decimalsResult; + +if (decimals < 0 || decimals > 18) { + throw new Error('Invalid decimals value'); } const value = parseUnits(params.value, decimals);
379-400: Add specific error handling for common ERC20 transfer failuresThe function should handle common ERC20 transfer failures more specifically to provide better error messages to users.
async function handleErc20Transfer( params: TransactionParams, contractAddress: Address, ): Promise<Address> { + try { const ABItoUse = contractAddress === '0xdac17f958d2ee523a2206206994597c13d831ec7' ? usdtMainnetABI : erc20Abi; const baseProps = { address: contractAddress, abi: ABItoUse, }; // ... rest of the function ... + } catch (error: any) { + // Handle specific ERC20 errors + if (error.message?.includes('insufficient allowance')) { + throw new Error('Insufficient token allowance'); + } + if (error.message?.includes('insufficient balance')) { + throw new Error('Insufficient token balance'); + } + // Re-throw other errors + throw error; + } }lang/ca.json (1)
101-102: The new entries should be translated to CatalanThe new entries "Q/acc" and "Q/acc News" are in English, while this is a Catalan localization file. Consider translating these entries to maintain consistency with the rest of the translations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
lang/ca.json(1 hunks)lang/en.json(1 hunks)lang/es.json(1 hunks)package.json(1 hunks)src/artifacts/usdtMainnetABI.json(1 hunks)src/components/Footer/Footer.tsx(2 hunks)src/components/views/donate/OneTime/SelectTokenModal/SelectTokenModal.tsx(1 hunks)src/components/views/givfarm/GIVfarmBottom.tsx(1 hunks)src/config/production.tsx(1 hunks)src/hooks/useCreateEvmDonation.tsx(2 hunks)src/hooks/usePassport.ts(1 hunks)src/lib/constants/links.ts(1 hunks)src/lib/helpers.ts(2 hunks)src/lib/transaction.ts(1 hunks)src/wagmiConfigs.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- package.json
- src/artifacts/usdtMainnetABI.json
- src/config/production.tsx
🔇 Additional comments (11)
src/wagmiConfigs.ts (2)
5-5: LGTM: Clean import addition
The import of mainnet from viem/chains is appropriate for the chain identification logic.
46-49: Verify the impact of using public transport for non-mainnet chains
The current implementation falls back to public transport for non-mainnet chains, which could lead to rate limiting or reliability issues in production.
Let's check for other chain configurations in the codebase:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the impact of using public transport for non-mainnet chains:
Using public transport for non-mainnet chains is an intentional and safe design choice
The implementation in wagmiConfigs.ts is actually following best practices because:
-
The code specifically checks for mainnet to use Infura, while other networks use their own dedicated RPC endpoints configured in their respective network configs (as seen in the search results showing RPC configurations for various chains).
-
The "public transport" for non-mainnet chains refers to the chain's own RPC endpoints (e.g., for Optimism, Gnosis, etc.) which are properly configured in the network configs and are production-ready.
-
The codebase shows careful handling of different networks with proper RPC configurations, including fallback providers and network-specific endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for chain configurations and RPC endpoints
echo "Searching for chain configurations..."
rg -l "chains.*=.*\[" --type ts
echo "Searching for RPC URL configurations..."
rg -l "rpc.*url" --type ts --type json
echo "Searching for Infura usage..."
rg -l "infura" --type ts --type json --type env
Length of output: 475
Script:
#!/bin/bash
# Let's search for chain configurations and HTTP transport setup more thoroughly
echo "Searching for chain and transport configurations..."
rg -A 5 "chain.*=|transport.*=|http\(.*\)" --type ts
echo "Searching for files containing mainnet or network configurations..."
rg -A 5 "mainnet|chain\.id" --type ts
echo "Searching for RPC related configurations..."
rg -A 5 "rpc|provider" --type ts
Length of output: 211090
src/lib/constants/links.ts (2)
59-60: LGTM! New QACC constants follow existing patterns.
The new QACC-related constants are well-structured and follow the established naming conventions.
59-60: Verify Footer component integration.
Let's verify that these new links are properly integrated into the Footer component with correct localization.
✅ Verification successful
Links are properly integrated with localization support
The QACC links are correctly integrated in the Footer component with:
- Proper usage of
links.QACCandlinks.QACC_NEWSconstants - Translation keys
component.title.qaccandcomponent.title.qacc_newsimplemented in multiple language files (en, es, ca) - Correct implementation of
formatMessagefor localization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Footer component integration and localization
# Check Footer component for QACC links usage
echo "Checking Footer component for QACC links..."
rg -A 2 "QACC|QACC_NEWS" "src/components/Footer"
# Verify localization keys exist
echo "Verifying localization entries..."
rg -A 1 "qacc" "lang/*.json"
Length of output: 719
Script:
#!/bin/bash
# Let's check for localization files in the correct directory structure
echo "Finding localization files..."
fd -e json -d 3 "en|es|de"
# Once we find the location, let's check for QACC related translations
echo "Checking translations in public directory..."
rg -A 1 "qacc" "public/locales/"
# Let's also verify the import and usage pattern in Footer
echo "Checking Footer component structure..."
cat "src/components/Footer/Footer.tsx"
Length of output: 8185
Script:
#!/bin/bash
# Let's check for QACC related translations in the lang directory
echo "Checking translations in lang directory..."
rg -A 1 "qacc" "lang/"
# Let's also check the translation IDs used in Footer for QACC
echo "Checking specific translation keys used..."
rg "id: 'component.title.qacc" "src/components/Footer/Footer.tsx"
Length of output: 862
src/components/views/givfarm/GIVfarmBottom.tsx (2)
191-191: LGTM: Simplified pool rendering structure
The removal of the extra Col wrapper simplifies the component structure. The grid layout is still maintained through the renderPool function which wraps each pool with appropriate Col components.
191-191: Verify StakeTogetherCard removal impact
The removal of StakeTogetherCard from this component might affect staking functionality. Please ensure this feature is properly relocated or intentionally removed.
✅ Verification successful
Let me gather more information about the component's usage and potential relocation.
Let me check where this component is being used and how it's being rendered in the application.
Based on the git history and code changes, I can now provide a definitive response about the StakeTogetherCard removal.
StakeTogetherCard removal is intentional and safe
The removal of StakeTogetherCard from GIVfarmBottom was an intentional change (commit 9f67e61) that aligns with the codebase's evolution. The component was:
- First added on April 10, 2024 (commit 59c04ab)
- Actively maintained with features like conditional links and translations
- Deliberately removed on October 23, 2024 (commit 9f67e61)
The removal is safe because:
- The component still exists in the codebase at its original location (
src/components/cards/StakingCards/BaseStakingCard/StakeTogetherCard.tsx) - The removal was a targeted change to only remove it from the GIVfarm view
- No other components were depending on its presence in GIVfarmBottom
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StakeTogetherCard usage in the codebase
echo "Checking if StakeTogetherCard is used elsewhere:"
rg "StakeTogetherCard" -l
# Search for any new staking-related components that might replace it
echo -e "\nChecking for potential replacement components:"
rg -i "stake.*card" -l
Length of output: 719
Script:
#!/bin/bash
# Check the git history for StakeTogetherCard changes
echo "Checking recent changes to StakeTogetherCard:"
git log -p -- src/components/cards/StakingCards/BaseStakingCard/StakeTogetherCard.tsx
# Check for imports of StakeTogetherCard
echo -e "\nChecking for StakeTogetherCard imports:"
rg "import.*StakeTogetherCard" -A 1
# Check the content of StakeTogetherCard component
echo -e "\nChecking StakeTogetherCard component content:"
cat src/components/cards/StakingCards/BaseStakingCard/StakeTogetherCard.tsx
Length of output: 9063
Script:
#!/bin/bash
# Check for StakeTogetherCard imports and usage
echo "Checking for StakeTogetherCard imports and usage:"
rg "StakeTogetherCard" -B 2 -A 2
# Check the GIVfarmBottom component's history
echo -e "\nChecking GIVfarmBottom recent changes:"
git log -p -- src/components/views/givfarm/GIVfarmBottom.tsx
Length of output: 76058
src/components/Footer/Footer.tsx (1)
244-244: Verify the layout impact of gap change
The gap property change from what appears to be 72px to 0px in the laptopL media query might affect the spacing between link columns in the footer.
Please verify that this change:
- Doesn't cause overlapping of link columns
- Maintains consistent spacing with other footer elements
- Looks correct across different screen sizes
Consider providing screenshots of the footer layout before and after this change to confirm the visual impact.
src/hooks/useCreateEvmDonation.tsx (1)
21-21:
Potential type safety issue with txHash assertion
The broadened type of txHash to include Address combined with the forced type assertion to 0x${string} in useWaitForTransactionReceipt could lead to runtime errors if txHash is actually an Address.
Consider adding a type guard:
-hash: txHash as `0x${string}`,
+hash: txHash?.startsWith('0x') ? txHash as `0x${string}` : undefined,Also applies to: 33-33
src/lib/helpers.ts (1)
22-22: Verify the USDT mainnet ABI file
Let's ensure the imported ABI file exists and contains the correct USDT contract interface.
✅ Verification successful
USDT Mainnet ABI file verified successfully
The ABI file exists at the expected location and contains all the required ERC20 functions (transfer, balanceOf, decimals) needed for USDT token operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify USDT mainnet ABI file existence and content
# Check if the ABI file exists
if [ -f "src/artifacts/usdtMainnetABI.json" ]; then
echo "File exists. Checking content..."
# Verify it's a valid JSON and contains essential USDT functions
jq -e '
. as $abi |
["transfer", "balanceOf", "decimals"] as $required |
$required - [($abi[] | select(.type == "function") | .name)] | length == 0
' src/artifacts/usdtMainnetABI.json
else
echo "ABI file not found at expected location"
# Search for the file in case it's in a different location
fd -t f "usdtMainnetABI.json"
fi
Length of output: 340
lang/en.json (1)
100-101: LGTM! The new Q/acc localization entries are properly formatted.
The new entries follow the established naming conventions and are correctly placed in alphabetical order with other component titles.
lang/es.json (1)
709-710: LGTM! The new Q/acc related translations are properly added.
The new entries follow the established patterns for component titles and appropriately preserve the branded terms without translation.
| // TODO: we must manage this for all chains in a better way | ||
| // basically to stop using viem's public transport | ||
| // leaving this as a hotfix to keep it quick | ||
| const infuraKey = process.env.NEXT_PUBLIC_INFURA_API_KEY; |
There was a problem hiding this comment.
Add environment variable validation and optimize transport creation
Several improvements are needed for production readiness:
- The Infura key should be validated at startup
- The HTTP transport instance could be memoized
Consider this implementation:
- const infuraKey = process.env.NEXT_PUBLIC_INFURA_API_KEY;
+ const infuraKey = process.env.NEXT_PUBLIC_INFURA_API_KEY;
+ if (!infuraKey) {
+ console.warn('Infura API key not found, falling back to public transport');
+ }
+
+ // Memoize transports
+ const mainnetTransport = infuraKey ? http(`https://mainnet.infura.io/v3/${infuraKey}`) : http();
+ const defaultTransport = http();
+
const customHttp =
chain.id === mainnet.id && infuraKey
- ? http(`https://mainnet.infura.io/v3/${infuraKey}`)
- : http();
+ ? mainnetTransport
+ : defaultTransport;Also applies to: 46-51
| <a href={links.QACC} target='_blank'> | ||
| <LinkItem color={textColor}> | ||
| {formatMessage({ | ||
| id: 'component.title.qacc', | ||
| })} | ||
| </LinkItem> | ||
| </a> |
There was a problem hiding this comment.
Add security attributes to external links
The new external links should include rel="noreferrer noopener" attributes to prevent potential security vulnerabilities when using target="_blank".
Apply this diff:
- <a href={links.QACC} target='_blank'>
+ <a href={links.QACC} target='_blank' rel='noreferrer noopener'>
- <a href={links.QACC_NEWS} target='_blank'>
+ <a href={links.QACC_NEWS} target='_blank' rel='noreferrer noopener'>Also applies to: 153-159
| // 'viem' ABI contract for USDT donation fails on mainnet | ||
| // so we use the USDT mainnet ABI instead and put inside usdtMainnetABI.json file | ||
| // update for 'viem' package to fix this issue doesn't work | ||
| const ABItoUse = | ||
| contractAddress === '0xdac17f958d2ee523a2206206994597c13d831ec7' | ||
| ? usdtMainnetABI | ||
| : erc20Abi; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move the USDT contract address to constants
The hardcoded USDT contract address should be moved to a constants file for better maintainability and reusability.
+// In constants.ts
+export const USDT_MAINNET_ADDRESS = '0xdac17f958d2ee523a2206206994597c13d831ec7' as const;
-const ABItoUse =
- contractAddress === '0xdac17f958d2ee523a2206206994597c13d831ec7'
+const ABItoUse =
+ contractAddress === USDT_MAINNET_ADDRESSCommittable suggestion skipped: line range outside the PR's diff.
Fix recurring donations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/components/views/userProfile/donationsTab/recurringTab/StreamRow.tsx (3)
22-28: LGTM! Consider adding error handling.The memoization of superToken lookup is a good performance optimization. However, consider adding a fallback for cases where the token is not found in the config.
const superToken = useMemo( () => config.OPTIMISM_CONFIG.SUPER_FLUID_TOKENS.find( s => s.id === tokenStream[0].token.id, - ), + ) ?? null, [tokenStream], );
67-67: Add a fallback for undefined symbol.While the optional chaining is good, consider adding a fallback value for better user experience when the symbol is undefined.
-<P>{superToken?.symbol}</P> +<P>{superToken?.symbol ?? 'Unknown Token'}</P>
Line range hint
1-141: Consider extracting business logic to custom hooks.The component currently handles both UI rendering and complex business logic. Consider extracting the following into custom hooks for better maintainability and reusability:
- Token balance and flow rate calculations
- Chain switching logic
- Super token lookup and validation
Example structure:
// useStreamCalculations.ts export const useStreamCalculations = (tokenStream: ISuperfluidStream[]) => { const totalFlowRate = useMemo(() => tokenStream.reduce((acc, curr) => acc + BigInt(curr.currentFlowRate), 0n), [tokenStream] ); // ... other calculations return { totalFlowRate, monthlyFlowRate, runOutMonth }; }; // In StreamRow.tsx const { totalFlowRate, monthlyFlowRate, runOutMonth } = useStreamCalculations(tokenStream);src/components/views/donate/Recurring/RecurringDonationModal/RecurringDonationModal.tsx (1)
231-252: Consider generalizing token decimal conversions for tokens with varying decimalsCurrently, the code handles tokens with 6 decimals by converting amounts to 18 decimals. To enhance flexibility and future-proof the code, consider generalizing this logic to handle tokens with any number of decimals by adjusting amounts based on the difference between the token's decimals and the standard 18 decimals.
src/components/views/donate/Recurring/ModifySuperToken/DepositSuperToken.tsx (1)
137-143: Handling tokens with 6 decimals by adjusting amountThe added logic converts the
amountto 18 decimals if the token has 6 decimals, ensuring compatibility with the Superfluid upgrade operation. Consider generalizing this to handle tokens with any number of decimals.src/components/views/donate/Recurring/SelectTokenModal/SelectTokenModal.tsx (2)
88-90: Consider using case-insensitive comparison for token symbolsThe token filtering logic uses case-sensitive comparison which could lead to matching issues. Consider converting symbols to lowercase for comparison.
const filteredTokens = superTokens.filter(token => { - return !(newBalances[token.underlyingToken.symbol] > 0n); + return !(newBalances[token.underlyingToken.symbol.toLowerCase()] > 0n); });
192-223: Simplify the underlying tokens rendering logicThe Fragment wrapper (
<>) is unnecessary since we're already mapping over the tokens. Also, the key prop should be moved to theTokenInfocomponent.{underlyingTokens.length > 0 ? ( underlyingTokens.map(token => ( - <> <TokenInfo - key={token.underlyingToken?.symbol} + key={token.underlyingToken.symbol} token={token.underlyingToken} balance={balances[token.underlyingToken.symbol]} disable={ balances[token.underlyingToken.symbol] === undefined || balances[token.underlyingToken.symbol] === 0n } onClick={() => { setSelectedRecurringToken({ token: token.underlyingToken, balance: balances[token.underlyingToken.symbol], }); setShowModal(false); }} /> - </> )) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
public/images/tokens/cbBTC.pngis excluded by!**/*.pngpublic/images/tokens/cbBTC.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
src/components/views/donate/Recurring/ModifySuperToken/DepositSuperToken.tsx(2 hunks)src/components/views/donate/Recurring/ModifySuperToken/ModifySuperTokenModal.tsx(1 hunks)src/components/views/donate/Recurring/ModifySuperToken/WithDrawSuperToken.tsx(1 hunks)src/components/views/donate/Recurring/RecurringDonationCard.tsx(3 hunks)src/components/views/donate/Recurring/RecurringDonationModal/Item.tsx(2 hunks)src/components/views/donate/Recurring/RecurringDonationModal/RecurringDonationModal.tsx(4 hunks)src/components/views/donate/Recurring/SelectTokenModal/SelectTokenModal.tsx(5 hunks)src/components/views/userProfile/donationsTab/recurringTab/StreamRow.tsx(6 hunks)src/config/production.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/production.tsx
🔇 Additional comments (17)
src/components/views/userProfile/donationsTab/recurringTab/StreamRow.tsx (2)
119-126: LGTM! Good type safety improvement.
The addition of the superToken existence check before rendering the modal and passing it as a prop improves type safety and prevents potential runtime errors.
71-72: Verify decimal places handling for all super tokens.
The code assumes 18 decimals for formatting the monthly flow rate. This might not be correct for all super tokens.
src/components/views/donate/Recurring/RecurringDonationModal/RecurringDonationModal.tsx (4)
13-13: Importing ethers library for token amount conversions
The addition of import { ethers } from 'ethers'; is necessary for handling token amount conversions using ethers.utils.parseUnits in the code below.
256-256: Updating upgrade operation with adjusted amount
Using newAmount.toString() in the upgrade operation ensures that the correct, decimal-adjusted token amount is utilized during the upgrade process.
271-272: Flow rate calculation uses adjusted token amount
The flow rate _flowRate is calculated using newPerMonthAmount, which accounts for tokens with different decimals. This ensures accurate flow rates for tokens after decimal adjustment.
305-305: Calculating Giveth donation flow rate with adjusted amount
The calculation of Giveth's flow rate using newPerMonthAmount correctly accounts for tokens with adjusted decimals, ensuring accurate donation amounts.
src/components/views/donate/Recurring/RecurringDonationModal/Item.tsx (3)
32-32: Accurate formatting of token amounts using token.decimals
By using token.decimals directly in formatUnits, the amount is formatted accurately based on the token's actual decimal precision.
34-34: Correct token symbol resolution
The expression {token.underlyingToken?.symbol || token.symbol} ensures that the underlying token symbol is displayed if available, otherwise it falls back to the super token's symbol.
47-47: Consistent use of token.decimals in USD value calculation
Using token.decimals in formatting the USD value maintains consistency and accuracy in the displayed amounts.
src/components/views/donate/Recurring/ModifySuperToken/ModifySuperTokenModal.tsx (1)
16-16: Updating selectedToken type for enhanced flexibility
Changing the type of selectedToken to ISuperToken | IToken allows the modal to handle both super tokens and regular tokens, improving the component's flexibility.
src/components/views/donate/Recurring/ModifySuperToken/WithDrawSuperToken.tsx (1)
190-190: Passing superToken to Item component
Updating the Item component to receive superToken instead of token ensures that the correct token properties are used when displaying withdrawal information.
src/components/views/donate/Recurring/ModifySuperToken/DepositSuperToken.tsx (2)
5-5: Importing ethers library for token amount conversions
The import of ethers is necessary to utilize ethers.utils.parseUnits for converting token amounts based on decimal precision.
147-147: Using adjusted amount in upgrade operation
Updating the upgrade method to use newAmount.toString() ensures the correct, decimal-adjusted amount is used during the token upgrade process.
src/components/views/donate/Recurring/SelectTokenModal/SelectTokenModal.tsx (1)
132-134: LGTM! Good use of case-insensitive comparison
The case-insensitive comparison for token IDs is a good practice to prevent matching issues.
src/components/views/donate/Recurring/RecurringDonationCard.tsx (3)
145-148: LGTM! Good handling of token decimals
The introduction of special handling for 6 decimal tokens and the scaling factor is a good practice to ensure accurate calculations across different token standards.
Also applies to: 153-156
158-158: LGTM! Proper scaling of calculations
The calculations for totalPerSec and totalStreamPerMonth correctly use the scaling factor to maintain precision.
Also applies to: 180-181
163-164: LGTM! Consistent case-insensitive token handling
The use of lowercase for token IDs is consistent with the approach in SelectTokenModal, preventing case-sensitivity issues.
Also applies to: 780-782
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/views/donate/Recurring/SelectTokenModal/StreamInfo.tsx (1)
Line range hint
39-41: Remove redundant token lookupThe
findTokenByAddresscall seems unnecessary since we now have thesuperTokenprop that contains the token information.-const token = findTokenByAddress(stream[0].token.id); -const underlyingToken = token?.underlyingToken; +const underlyingToken = superToken.underlyingToken;src/components/views/donate/Recurring/SelectTokenModal/SelectTokenModal.tsx (2)
88-90: Simplify token filtering conditionThe current filtering condition can be simplified for better readability.
const filteredTokens = superTokens.filter(token => { - return !(newBalances[token.underlyingToken.symbol] > 0n); + return !newBalances[token.underlyingToken.symbol]; });
54-54: Consider memoizing underlyingTokensSince
underlyingTokensis derived from the constantsuperTokens, consider usinguseMemoto prevent unnecessary re-renders.-const [underlyingTokens, setUnderlyingTokens] = useState<ISuperToken[]>([]); +const underlyingTokens = useMemo(() => superTokens, []); // Remove this line -setUnderlyingTokens(superTokens);Also applies to: 93-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/views/donate/Recurring/RecurringDonationCard.tsx(5 hunks)src/components/views/donate/Recurring/SelectTokenModal/SelectTokenModal.tsx(5 hunks)src/components/views/donate/Recurring/SelectTokenModal/StreamInfo.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/views/donate/Recurring/RecurringDonationCard.tsx
🔇 Additional comments (2)
src/components/views/donate/Recurring/SelectTokenModal/StreamInfo.tsx (1)
14-14: LGTM: Props interface enhancement
The addition of the superToken prop improves type safety and makes the token information flow more explicit.
Also applies to: 24-24
src/components/views/donate/Recurring/SelectTokenModal/SelectTokenModal.tsx (1)
131-133: LGTM: Case-insensitive token ID comparison
The case-insensitive comparison is a good defensive programming practice that prevents issues with token ID casing mismatches.
| {underlyingTokens.length > 0 ? ( | ||
| underlyingTokens.map(token => ( | ||
| <> | ||
| <TokenInfo | ||
| key={token.underlyingToken?.symbol} | ||
| token={token.underlyingToken} | ||
| balance={ | ||
| balances[ | ||
| token.underlyingToken.symbol | ||
| ] | ||
| } | ||
| disable={ | ||
| balances[ | ||
| token.underlyingToken.symbol | ||
| ] === undefined || | ||
| balances[ | ||
| token.underlyingToken.symbol | ||
| ] === 0n | ||
| } | ||
| onClick={() => { | ||
| setSelectedRecurringToken({ | ||
| token: token.underlyingToken, | ||
| balance: | ||
| balances[ | ||
| token.underlyingToken | ||
| .symbol | ||
| ], | ||
| }); | ||
| setShowModal(false); | ||
| }} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
Fix Fragment usage and key placement
The current implementation has several issues:
- Unnecessary Fragment wrapping
- Key prop on Fragment instead of TokenInfo
- Deeply nested balance access pattern
{underlyingTokens.length > 0 ? (
underlyingTokens.map(token => (
- <>
<TokenInfo
- key={token.underlyingToken?.symbol}
+ key={token.underlyingToken.id}
token={token.underlyingToken}
- balance={balances[token.underlyingToken.symbol]}
+ balance={balances[token.underlyingToken.symbol] || 0n}
disable={
- balances[token.underlyingToken.symbol] === undefined ||
- balances[token.underlyingToken.symbol] === 0n
+ !balances[token.underlyingToken.symbol]
}
onClick={() => {
setSelectedRecurringToken({
token: token.underlyingToken,
- balance: balances[token.underlyingToken.symbol],
+ balance: balances[token.underlyingToken.symbol] || 0n,
});
setShowModal(false);
}}
/>
- </>
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {underlyingTokens.length > 0 ? ( | |
| underlyingTokens.map(token => ( | |
| <> | |
| <TokenInfo | |
| key={token.underlyingToken?.symbol} | |
| token={token.underlyingToken} | |
| balance={ | |
| balances[ | |
| token.underlyingToken.symbol | |
| ] | |
| } | |
| disable={ | |
| balances[ | |
| token.underlyingToken.symbol | |
| ] === undefined || | |
| balances[ | |
| token.underlyingToken.symbol | |
| ] === 0n | |
| } | |
| onClick={() => { | |
| setSelectedRecurringToken({ | |
| token: token.underlyingToken, | |
| balance: | |
| balances[ | |
| token.underlyingToken | |
| .symbol | |
| ], | |
| }); | |
| setShowModal(false); | |
| }} | |
| /> | |
| </> | |
| {underlyingTokens.length > 0 ? ( | |
| underlyingTokens.map(token => ( | |
| <TokenInfo | |
| key={token.underlyingToken.id} | |
| token={token.underlyingToken} | |
| balance={balances[token.underlyingToken.symbol] || 0n} | |
| disable={!balances[token.underlyingToken.symbol]} | |
| onClick={() => { | |
| setSelectedRecurringToken({ | |
| token: token.underlyingToken, | |
| balance: balances[token.underlyingToken.symbol] || 0n, | |
| }); | |
| setShowModal(false); | |
| }} | |
| /> | |
| )) |
|
@lovelgeorge99 we can close this one? We merged main to develop this morning. thx |
Summary by CodeRabbit
Release Notes
New Features
/comarketingto the Giveth Co-Marketing Slides PDF.Bug Fixes
Improvements
Chores
package.jsonfor improved stability.