fix(request): correctly identify native tokens #434
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
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 (6)
src/utils/fetch.utils.ts (2)
62-64: Approve changes with a suggestion for improvementThe refactoring to use
utils.isAddressZero(tokenAddress)is a good improvement. It enhances code readability and centralizes the zero address check logic, which aligns with the PR objectives.However, consider extracting the hardcoded address '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' into a named constant for better maintainability and clarity. This could be defined at the module level or imported from a constants file if used elsewhere.
Consider applying this change:
+ const ALTERNATIVE_ZERO_ADDRESS = '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee'; export const fetchTokenPrice = async ( // ... (rest of the function remains the same) body: JSON.stringify({ tokenAddress: utils.isAddressZero(tokenAddress) - ? '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' + ? ALTERNATIVE_ZERO_ADDRESS : tokenAddress, chainId, }), // ... (rest of the function remains the same)This change would improve code readability and make it easier to update the alternative address if needed in the future.
Line range hint
29-94: Consider enhancing error handling and API response processingWhile the main changes look good, here are some suggestions to further improve the function:
Error Handling: The current error handling only logs to the console. Consider adding more robust error handling, such as returning an error object or throwing a custom error that can be caught and handled by the caller.
API Response Validation: The function assumes the existence of certain properties in the API response. Consider adding checks for these properties to prevent potential runtime errors if the API response structure changes.
Stablecoin Check: The use of
utils.estimateStableCoinis not immediately clear. Consider adding a comment explaining the purpose and logic of this check.Here's an example of how you might implement these suggestions:
export const fetchTokenPrice = async ( tokenAddress: string, chainId: string, host?: string ): Promise<ITokenPriceData | { error: string }> => { try { // ... (existing code) if (mobulaResponse.ok) { const data = json.data; if (!data || typeof data !== 'object') { throw new Error('Invalid API response structure'); } const decimals = data.decimals ?? data.contracts?.find((contract) => contract.blockchainId === chainId)?.decimals; if (decimals === undefined) { throw new Error('Unable to determine token decimals'); } let tokenData = { price: data.price, chainId: chainId, address: tokenAddress, name: data.name, symbol: data.symbol, decimals, logoURI: data.logo, }; // Check if the token is a stablecoin and adjust price if necessary if (utils.estimateStableCoin(data.price)) { tokenData.price = 1; } return tokenData; } else { return { error: `API request failed with status ${mobulaResponse.status}` }; } } catch (error) { console.error('Error fetching token price:', error); return { error: `Failed to fetch token price for ${tokenAddress} on chain ${chainId}` }; } }This implementation includes more robust error handling, API response validation, and a comment explaining the stablecoin check. It also changes the return type to include a possible error object, allowing the caller to handle errors more effectively.
src/hooks/useBalance.tsx (2)
4-4: LGTM! Consider using named imports for better clarity.The addition of the utility import aligns with the PR objectives and improves code organization. However, consider using named imports for better clarity and to potentially benefit from tree-shaking.
You could refactor the import statement as follows:
-import * as utils from '@/utils' +import { isAddressZero } from '@/utils'This change would make it clear which specific utility functions are being used in this file.
126-128: LGTM! Consider using consistent return style for readability.The refactoring to use
utils.isAddressZero()improves code clarity and maintainability. It aligns well with the PR objectives.For consistency and improved readability, consider using explicit return statements:
-if (utils.isAddressZero(a.address)) - return -1 -if (utils.isAddressZero(b.address)) - return 1 +if (utils.isAddressZero(a.address)) { + return -1; +} +if (utils.isAddressZero(b.address)) { + return 1; +}This change makes the code style more consistent with the rest of the function.
src/components/Request/Pay/Views/Initial.view.tsx (2)
Line range hint
273-292: Refactor to eliminate duplicated code inhandleOnNextfunctionThe
handleOnNextfunction contains duplicated code in both theif (!isXChain)andelseblocks, such as submitting the request fulfillment, saving fulfillment details, and updating state. This duplication increases maintenance effort and the risk of inconsistencies. Consider refactoring the shared logic into a separate function to improve maintainability and reduce redundancy.Apply this refactor to extract common code:
const handleOnNext = async () => { const amountUsd = (Number(requestLinkData.tokenAmount) * (tokenPriceData?.price ?? 0)).toFixed(2) try { setErrorState({ showError: false, errorMessage: '' }) if (!unsignedTx) return if (!isXChain) { await checkUserHasEnoughBalance({ tokenValue: requestLinkData.tokenAmount }) if (selectedTokenData?.chainId !== String(currentChain?.id)) { await switchNetwork(selectedTokenData!.chainId) } setLoadingState('Sign in wallet') const hash = await sendTransactions({ preparedDepositTxs: { unsignedTxs: [unsignedTx] }, feeOptions: undefined, }) - setLoadingState('Executing transaction') - await peanut.submitRequestLinkFulfillment({ - chainId: requestLinkData.chainId, - hash: hash ?? '', - payerAddress: address ?? '', - link: requestLinkData.link, - apiUrl: '/api/proxy/patch/', - amountUsd, - }) - const currentDate = new Date().toISOString() - utils.saveRequestLinkFulfillmentToLocalStorage({ - details: { - ...requestLinkData, - destinationChainFulfillmentHash: hash ?? '', - createdAt: currentDate, - }, - link: requestLinkData.link, - }) - setTransactionHash(hash ?? '') - onNext() } else { await checkUserHasEnoughBalance({ tokenValue: estimatedFromValue }) if (selectedTokenData!.chainId !== String(currentChain?.id)) { await switchNetwork(selectedTokenData!.chainId) } setLoadingState('Sign in wallet') const xchainUnsignedTxs = await createXChainUnsignedTx() const { unsignedTxs } = xchainUnsignedTxs const hash = await sendTransactions({ preparedDepositTxs: { unsignedTxs }, feeOptions: undefined, }) - setLoadingState('Executing transaction') - await peanut.submitRequestLinkFulfillment({ - chainId: requestLinkData.chainId, - hash: hash ?? '', - payerAddress: address ?? '', - link: requestLinkData.link, - apiUrl: '/api/proxy/patch/', - amountUsd, - }) - const currentDate = new Date().toISOString() - utils.saveRequestLinkFulfillmentToLocalStorage({ - details: { - ...requestLinkData, - destinationChainFulfillmentHash: hash ?? '', - createdAt: currentDate, - }, - link: requestLinkData.link, - }) - setTransactionHash(hash ?? '') - onNext() } + // Common code extracted from both branches + setLoadingState('Executing transaction') + await peanut.submitRequestLinkFulfillment({ + chainId: requestLinkData.chainId, + hash: hash ?? '', + payerAddress: address ?? '', + link: requestLinkData.link, + apiUrl: '/api/proxy/patch/', + amountUsd, + }) + const currentDate = new Date().toISOString() + utils.saveRequestLinkFulfillmentToLocalStorage({ + details: { + ...requestLinkData, + destinationChainFulfillmentHash: hash ?? '', + createdAt: currentDate, + }, + link: requestLinkData.link, + }) + setTransactionHash(hash ?? '') + onNext() } catch (error) { const errorString = utils.ErrorHandler(error) setErrorState({ showError: true, errorMessage: errorString, }) console.error('Error while submitting request link fulfillment:', error) } finally { setLoadingState('Idle') } }Also applies to: 305-324
Line range hint
97-101: Add error handling infetchTokenSymbolfunctionThe
fetchTokenSymbolfunction does not handle errors that may occur when fetching token contract details. If an error occurs, it could lead to unhandled promise rejections or runtime errors. Consider adding atry...catchblock to handle potential errors and ensure the application remains stable.Apply this diff to implement error handling:
const fetchTokenSymbol = async (chainId: string, address: string) => { + try { const provider = await peanut.getDefaultProvider(chainId) const tokenContract = await peanut.getTokenContractDetails({ address, provider, }) setTokenRequestedSymbol(tokenContract?.symbol ?? '') + } catch (error) { + console.error('Failed to fetch token symbol:', error) + setTokenRequestedSymbol('') + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/components/Request/Pay/Views/Initial.view.tsx (3 hunks)
- src/components/Request/Pay/utils.ts (0 hunks)
- src/hooks/useBalance.tsx (2 hunks)
- src/utils/fetch.utils.ts (1 hunks)
- src/utils/general.utils.ts (1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Request/Pay/utils.ts
🧰 Additional context used
🔇 Additional comments (5)
src/utils/fetch.utils.ts (1)
Line range hint
1-94: Summary of review for src/utils/fetch.utils.tsThe changes made to the
fetchTokenPricefunction align well with the PR objectives, particularly in refactoring the zero address comparison. The use ofutils.isAddressZeroimproves code readability and maintainability.Key points:
- The main change effectively addresses the issue of correctly identifying native tokens.
- The refactoring eliminates the need for a separate utility file, as mentioned in the PR objectives.
- There are opportunities for further improvements in error handling, API response validation, and code clarity.
Overall, the changes are a step in the right direction. Implementing the suggested improvements would further enhance the robustness and maintainability of this function.
src/hooks/useBalance.tsx (1)
Line range hint
1-180: Overall, the changes look good and align with the PR objectives.The modifications to
useBalance.tsximprove code organization and readability without altering the core functionality. The refactoring of zero address comparisons and the centralization of utility functions are positive changes that enhance maintainability.A few minor suggestions were made for further improvements, but these are not critical. The changes successfully address the issue of correctly identifying native tokens as mentioned in the PR objectives.
src/utils/general.utils.ts (3)
328-333: LGTM: Improved address comparison usingethers.constants.AddressZeroThe changes to
areTokenAddressesEqualfunction enhance code maintainability by usingethers.constants.AddressZeroinstead of hardcoded addresses. This is a good practice that reduces the risk of errors and improves readability.
336-338: LGTM: New utility functionisAddressZeroaddedThe new
isAddressZerofunction is a valuable addition that provides a clear and reusable way to check if an address is the zero address. It effectively leverages the existingareTokenAddressesEqualfunction, promoting code reuse.
336-338: 🛠️ Refactor suggestionConsider refactoring zero address checks across the codebase
With the introduction of the
isAddressZeroutility function, we have an opportunity to standardize and simplify zero address checks throughout the codebase. This can improve consistency and readability.Consider replacing direct comparisons to zero addresses or uses of
areTokenAddressesEqualfor zero address checks with this new utility function. For example:- if (address === '0x0000000000000000000000000000000000000000') + if (isAddressZero(address))or
- if (areTokenAddressesEqual(address, '0x0000000000000000000000000000000000000000')) + if (isAddressZero(address))✅ Verification successful
Zero address checks are already standardized across the codebase
Verified that there are no additional direct zero address comparisons outside the utility functions, ensuring consistency and improved readability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential places to use the new isAddressZero function rg --type typescript --type javascript -e "0x0{40}" -e "constants\.AddressZero" -e "areTokenAddressesEqual.*AddressZero"Length of output: 157
Script:
# Updated shell script to search for potential places to use the new isAddressZero function rg --glob "*.ts" --glob "*.js" -e "0x0{40}" -e "constants\.AddressZero" -e "areTokenAddressesEqual.*AddressZero"Length of output: 446
When fetching the price token we were inadvertently changing the token address, so then we did not recognized it as native. Fixed that
Also did some refactoring around the address zero comparison which also made unnecessary the individual utils file for Request/Pay