fix: req v2 token/balance realted bugs #720
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces substantial updates to payment-related components. The PaymentForm now fetches token prices asynchronously, calculates USD values, and enhances error handling by providing default values. The ConfirmPaymentView and PaymentPage components have been adjusted to format token amounts and recipient addresses correctly. Additionally, the URL parser now includes Binance Smart Chain variants, supports a fallback default chain, and its tests have been updated. Utility functions and global components have also been improved for more robust state management and UI interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant PF as PaymentForm
participant UE as useEffect Hook
participant FT as fetchTokenPrice
participant API as External API
participant UI as UI Renderer
PF->>UE: Mount component / requestDetails change
UE->>FT: Call fetchTokenPrice(tokenAddress, chainId)
FT->>API: Request token price
API-->>FT: Return token price
FT-->>UE: Return fetched token price
UE->>PF: Update requestedTokenPrice & isFetchingTokenPrice
PF->>UI: Render updated USD value and token amount
sequenceDiagram
participant PP as PaymentPage
participant FPU as fetchParsedURL
participant FA as formatAmount
participant D as Redux Dispatch
PP->>FPU: Parse payment URL
FPU->>FA: Format amount if present
FA-->>FPU: Return formatted amount
FPU->>D: Dispatch parsed payment data with formatted amount
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)src/components/Payment/PaymentForm/index.tsx (1)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 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 (
|
kushagrasarathe
changed the title
![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
🧹 Nitpick comments (3)
src/redux/types/payment.types.ts (1)
18-19: Added proper state properties for USD and token values.These new properties in the state will help fix the issue with correctly displaying USD values (TASK-9434) and support the fractional decimals fix (TASK-9455).
Consider adding documentation comments to clarify the purpose and expected format of these properties:
+// USD equivalent value of the token amount usdValue: string | null +// Formatted token amount with proper decimal places tokenValue: string | nullsrc/redux/slices/payment-slice.ts (1)
23-24: Initialized new state properties for USD and token values.The initial values are set as empty strings, while the type definition in
payment.types.tsallows forstring | null. Consider making these consistent.-usdValue: '', -tokenValue: '', +usdValue: null, +tokenValue: null,src/components/Payment/PaymentForm/index.tsx (1)
194-231: Implemented token price fetching functionality.This new useEffect correctly implements token price fetching for payment requests:
- Only runs when request details are available
- Properly manages loading state
- Calculates USD value from token amount and price
- Updates Redux store with both token and USD values
- Includes appropriate error handling
However, there's room for improvement in error feedback for users when price fetching fails.
Consider adding user-facing error feedback when token price fetching fails, instead of just logging to console:
} catch (error) { console.error('Error fetching token price:', error) + dispatch( + paymentActions.setError('Unable to fetch current token price. You can continue with the payment, but USD values may not be accurate.') + ) } finally { setIsFetchingTokenPrice(false) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Payment/PaymentForm/index.tsx(12 hunks)src/components/Payment/Views/Confirm.payment.view.tsx(5 hunks)src/lib/url-parser/types/payment.ts(1 hunks)src/lib/validation/amount.ts(2 hunks)src/redux/slices/payment-slice.ts(2 hunks)src/redux/types/payment.types.ts(1 hunks)
🔇 Additional comments (21)
src/lib/validation/amount.ts (2)
1-1: New import for consistent amount formatting.This import is part of the implementation to display numerical values with two significant digits after the decimal point (TASK-9314).
14-14: Good implementation of consistent amount formatting.Now the
validateAmountfunction not only validates the amount but also returns it properly formatted, which aligns with TASK-9314 for consistent display of numerical values with proper decimal precision.However, let's verify that all components using this function expect the formatted output:
#!/bin/bash # Find all usages of validateAmount rg "validateAmount" --type=ts --type=tsxsrc/lib/url-parser/types/payment.ts (1)
15-15:❓ Verification inconclusive
Enhanced chain type to include tokens list.
The modified type adds token information directly to the chain object, which helps resolve the issue with reading the token symbol mentioned in the PR.
Let's verify that all code that creates or uses the
ParsedURLobjects properly handles this enhanced type:
🏁 Script executed:
#!/bin/bash # Find usages of ParsedURL and check for potential issues with the updated type rg "ParsedURL" --type=ts --type=tsx -A 10 -B 10Length of output: 75
Action Item: Verify
ParsedURLUsages for Enhanced Chain Type HandlingThe type enhancement in
src/lib/url-parser/types/payment.ts(line 15) adds atokenslist to thechainobject (i.e.,chain?: interfaces.ISquidChain & { tokens: interfaces.ISquidToken[] }). This change is meant to address issues with reading the token symbol.A preliminary search for usages of
ParsedURLusing the following command encountered an error due to an unrecognized file type for TSX files:rg "ParsedURL" --type=ts --type=tsx -A 10 -B 10To correctly verify that all code creating or using
ParsedURLobjects accommodates the updated type, please re-run the search with a corrected command. For example:#!/bin/bash # Search in TypeScript (.ts) files rg "ParsedURL" --type=ts -A 10 -B 10 # Search in TSX files using glob pattern for *.tsx rg "ParsedURL" --glob="*.tsx" -A 10 -B 10Manually inspect the results to ensure that the enhanced chain type is properly handled across all usages.
src/redux/slices/payment-slice.ts (1)
61-64: Added reducer for setting payment values.This reducer properly updates both the USD and token values in the state, supporting the fix for displaying USD values correctly (TASK-9434).
Let's verify that this reducer is being called correctly from components:
#!/bin/bash # Find usages of setPaymentValues action rg "setPaymentValues" --type=ts --type=tsxsrc/components/Payment/PaymentForm/index.tsx (11)
22-30: Updated imports to enhance token price fetching and formatting.The imports now include
formatAmountandfetchTokenPrice, which align with the new token price fetching functionality and consistent formatting across the application.
50-52: New state variables added for token/USD value management.These new state variables properly support the USD conversion functionality:
usdValue: Stores the calculated USD equivalent of the token amountrequestedTokenPrice: Stores the fetched token priceisFetchingTokenPrice: Tracks the loading state during token price fetchingThis is a good implementation that separates concerns and maintains clear state management.
93-94: Fixed error handling in recipientTokenAddress.Good improvement to return a default empty string instead of throwing an error when the recipient type is invalid, making the component more resilient to edge cases.
107-108: Enhanced token symbol fallback handling.Improved error handling by returning 'USDC' as a default value when no token symbol is found, preventing potential runtime errors.
130-139: Improved decimals resolution logic with proper fallbacks.The token decimals resolution now follows a clear hierarchy with proper fallbacks:
- First checks the selected token data
- Then attempts to resolve from the token address and chain ID
- Falls back to a default value (6) if neither is available
This makes the code more robust against missing token data.
163-169: Enhanced chain setup with default token selection.When a chain is selected but no token is specified, the code now intelligently selects USDC as the default token when available. This improves the user experience by providing a sensible default.
174-176: Added decimals setup for token selection.The code now properly sets the selected token decimals when available from the token data, ensuring that the correct decimal precision is used throughout the payment process.
264-276: Added denomination-based token amount calculation.This new logic properly handles both TOKEN and USD denominations:
- For TOKEN input: Uses the input value directly
- For USD input: Converts to token amount using current price
The error check on line 270-272 is important to prevent invalid calculations when token price data is unavailable.
367-375: Improved payment details display with USD values.The code now displays both USD and token amounts when available, improving clarity for users. The formatting is consistent and user-friendly, showing USD value first when available.
427-435: Enhanced token amount and USD value synchronization.This updated useEffect properly keeps the token and USD values in sync when the input token amount changes:
- Updates display token amount
- Calculates and updates USD value when token price is available
- Clears USD value when price isn't available
This ensures that users always see accurate and up-to-date conversion information.
493-494: Added token price fetching to button loading state.The button now correctly shows loading state during token price fetching, which improves user experience by preventing interactions while data is being loaded.
src/components/Payment/Views/Confirm.payment.view.tsx (6)
17-18: Updated imports to use formatAmount for consistent formatting.The code now uses
formatAmountinstead of the previousformatTokenAmount, ensuring consistent number formatting across the application. This aligns with the changes in the PaymentForm component.
33-33: Updated store usage to include USD and token values.The component now correctly accesses the
usdValueandtokenValueproperties from the payment store, which are set in the PaymentForm component. This ensures consistency in the payment flow.
84-85: Fixed token amount precision for transactions.The code now correctly formats the token amount with the appropriate number of decimal places using
Number(requestLink.tokenAmount).toFixed(requestLink.tokenDecimals). This ensures that the token amount sent in the transaction has the correct precision.Also applies to: 90-90
141-173: Improved error handling for cross-chain transactions.The code now properly wraps the cross-chain transaction preparation in a try-catch block, providing better error handling and logging. This makes the cross-chain functionality more robust and helps with debugging.
The additional checks for
txData?.unsignedTxsand the extraction of fee-related information are also well-implemented.
440-440: Enhanced payment amount display with consistent formatting.The "You are paying" field now correctly uses
formatAmountto ensure consistent number formatting, improving readability and user experience.
445-446: Improved recipient label and received amount display.The code now intelligently adjusts the recipient label based on the recipient type, showing a shortened address for ADDRESS types and the identifier for other types. It also properly formats the received amount using the consistent
formatAmountfunction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/__tests__/url-parser.test.ts (2)
209-228: Test name no longer matches implementationThe test implementation has been updated to expect Arbitrum as the default chain when a token is provided without a chain, which is the correct behavior based on the code changes. However, the test name is descriptive rather than declarative, which makes it clear what the test is checking but doesn't assert what the expected behavior should be.
This test verifies the correct behavior but its name could be more specific about the assertion being made.
Consider renaming the test to more clearly reflect what you're testing:
-it('token without chain should return arbitrum as default chain', async () => { +it('should default to Arbitrum chain when token is provided without a chain', async () => {
230-236: Test could be improved for clarityThis test verifies parsing amount without token, but there's an inconsistency in the code comments vs. assertions. The comment mentions "Default token should be USDC on Arbitrum" but the assertion is checking that token symbol is undefined.
Consider updating either the comment or the assertion to make them consistent:
// Default token should be USDC on Arbitrum -expect(result.parsedUrl?.token?.symbol).toBe(undefined) +// No token is being set when only amount is provided +expect(result.parsedUrl?.token).toBe(undefined)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/[...recipient]/page.tsx(2 hunks)src/components/Global/WalletNavigation/index.tsx(1 hunks)src/lib/url-parser/parser.consts.ts(2 hunks)src/lib/url-parser/parser.ts(1 hunks)src/lib/validation/amount.ts(2 hunks)src/utils/__tests__/url-parser.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/validation/amount.ts
🔇 Additional comments (8)
src/components/Global/WalletNavigation/index.tsx (1)
54-58: Page refresh for active navigation links.This change adds a page refresh when users click on the currently active navigation link. This is a good usability enhancement that ensures data is up-to-date when users revisit pages they're already on.
While this implementation works well for refreshing data, consider whether this might impact user experience in cases where the page has unsaved form data or if the refresh causes significant load time. You might want to test this behavior across different parts of the application to ensure it doesn't disrupt user workflows.
src/lib/url-parser/parser.consts.ts (2)
1-1: Added BSC chain import.The addition of Binance Smart Chain (BSC) to the imports expands the chain support in the application.
11-11:✅ Verification successful
Added BSC to supported chains.
The Binance Smart Chain (BSC) has been added to the popular chain name variants with comprehensive aliases, which improves the parser's ability to recognize BSC-related requests.
🏁 Script executed:
#!/bin/bash # Check if the BSC chain is being used elsewhere in the codebase rg -A 2 -B 2 "bsc\\.id" --type ts --type tsxLength of output: 73
BSC Alias Addition Verification – Manual Confirmation Required
- Location: src/lib/url-parser/parser.consts.ts, line 11
- Change: The alias entry
correctly adds comprehensive Binance Smart Chain (BSC) variants.[bsc.id]: ['binance', 'bsc', 'bsc mainnet', 'binance smart chain', 'bnb chain', 'bnb'],- Verification Note: The initial verification script included
--type tsx, which caused a file type error. The above script limits the search to TypeScript (.ts) files only. Please run it manually and confirm that the expected output shows the BSC alias correctly.
🏁 Script executed:
#!/bin/bash # Rerun search for "bsc.id" exclusively in TypeScript files to avoid the tsx file type error. rg -A 2 -B 2 "bsc\.id" --type tsLength of output: 527
BSC Alias Addition Verified – No Further Changes Needed
- Location:
src/lib/url-parser/parser.consts.ts(line 11)- Verified Entry:
[bsc.id]: ['binance', 'bsc', 'bsc mainnet', 'binance smart chain', 'bnb chain', 'bnb'],- Summary: Comprehensive Binance Smart Chain aliases have been correctly added. The search confirmed the presence of the entry with no side effects or conflicts elsewhere in the codebase.
src/app/[...recipient]/page.tsx (2)
16-16: Added formatAmount utility import.The formatAmount utility is now imported to standardize display of amounts.
35-40:❓ Verification inconclusive
Improved amount formatting in parsed payment data.
The code now applies consistent formatting to the amount field when setting parsed payment data. This addresses issues with displaying fractional decimals (TASK-9455) and ensures numerical values are properly formatted.
Let's check how the formatAmount function is implemented to ensure it correctly handles all edge cases:
🏁 Script executed:
#!/bin/bash # Find the definition of formatAmount rg -A 5 "export function formatAmount" --type tsLength of output: 879
Action Required: Verify Implementation of
formatAmountAliasThe updated code now applies consistent formatting to the
amountfield, addressing fractional decimals (TASK-9455). However, our search in the repository revealed no direct definition for a function namedformatAmount—instead, we found two similar functions insrc/utils/general.utils.ts:
formatAmountWithDecimalsformatAmountWithSignificantDigitsPlease confirm that
formatAmountis correctly re-exported or aliased to one of these implementations so that it properly handles the intended edge cases. Additionally, verify that the passed arguments match the expected types used by the underlying formatting function.src/utils/__tests__/url-parser.test.ts (3)
362-381: Test implementation aligned with expected behaviorThe test correctly verifies that when a token is specified without a chain for an address recipient, the system defaults to using Arbitrum (chainId 42161) as the chain. This is consistent with the behavior defined in the other test cases.
83-94:✅ Verification successful
Verify that the PEANUT_WALLET_CHAIN constant is used consistently
The tests rely on the mocked PEANUT_WALLET_CHAIN constant to define Arbitrum as the default chain. This is a critical part of the implementation change being tested.
🏁 Script executed:
#!/bin/bash # Look for usages of PEANUT_WALLET_CHAIN in the codebase echo "Looking for usages of PEANUT_WALLET_CHAIN..." rg "PEANUT_WALLET_CHAIN" --type ts | grep -v "__tests__" echo "Looking for default chain handling in URL parser..." rg -A 10 "chainDetails =" --type ts | grep -v "__tests__"Length of output: 12188
PEANUT_WALLET_CHAIN Usage Verified – No Changes Required
After reviewing the test mock and its usage across the codebase:
- The constant is imported and used consistently in all relevant files (e.g., in URL parsing in
src/lib/url-parser/parser.tsand view components likesrc/components/Create/Link/Input.view.tsx).- The test mock correctly defines Arbitrum as the default chain, and the production code consistently uses
PEANUT_WALLET_CHAIN.idto derive chain details.- There is no actionable discrepancy between the intended behavior and the codebase usage.
481-490:✅ Verification successful
Verify potential regression in error handling
These error test cases still expect errors for invalid token/chain combinations. Make sure the URL parser's updated behavior of defaulting to Arbitrum doesn't inadvertently suppress legitimate error cases like these.
🏁 Script executed:
#!/bin/bash # Search for the implementation of parsePaymentURL function to verify error handling logic # Look for the function implementation echo "Searching for parsePaymentURL implementation..." rg -A 20 "parsePaymentURL" --type ts | grep -v "test" | head -n 50 # Look for error handling specifically for invalid tokens echo "Searching for INVALID_TOKEN error handling..." rg "INVALID_TOKEN" --type ts | grep -v "test" # Look for chain resolution logic echo "Searching for chain resolution logic..." rg "resolveChainId|getReadableChainName" --type ts | grep -v "test" | head -n 20Length of output: 5356
Ensure error handling remains robust with default chain behavior
After verifying the implementation and error test cases, the behavior inparsePaymentURL(located insrc/lib/url-parser/parser.ts) correctly returnsINVALID_TOKENandINVALID_CHAINerrors for invalid token/chain inputs—even with the updated default behavior. The tests insrc/utils/__tests__/url-parser.test.tsremain valid and ensure that the defaulting to Arbitrum does not suppress legitimate error cases.
- Confirmed that
parsePaymentURLreturns an error object when an invalid token or chain is provided.- Verified that error message constants such as
EParseUrlError.INVALID_TOKENare still used appropriately.- Checked that chain resolution logic (e.g., through
getReadableChainName) hasn’t overwritten error handling.
| if (!chainDetails && !isPeanutRecipient) { | ||
| // default to arb even for non-USERNAME recipients if no chain is specified | ||
| chainDetails = squidChainsAndTokens[PEANUT_WALLET_CHAIN.id] | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Added default chain fallback for non-USERNAME recipients.
This change ensures that even when no chain is specified for a non-USERNAME recipient, the system will default to a standard chain (likely Arbitrum based on PEANUT_WALLET_CHAIN.id). This improves the token resolution process and addresses the issue with reading token symbols mentioned in the PR.
It's important to ensure this default behavior is properly documented for users. Let's check if there's any documentation about chain selection in the application:
🏁 Script executed:
#!/bin/bash
# Check for documentation about default chain selection
fd README\.md --type f | xargs grep -l "chain" | xargs grep -l "default"
# Check where PEANUT_WALLET_CHAIN is defined to confirm it's Arbitrum
rg -A 2 "PEANUT_WALLET_CHAIN" --type tsLength of output: 10865
Default Chain Fallback – Update Documentation
- The fallback logic implemented in
src/lib/url-parser/parser.ts(lines 132–135) correctly assignschainDetailsfromsquidChainsAndTokensusingPEANUT_WALLET_CHAIN.id, which is confirmed to refer to Arbitrum. - Verification across the codebase confirms that this default behavior is intended for non-USERNAME recipients.
- However, our search did not reveal any explicit documentation (e.g., in
README.md) describing the default chain selection behavior. For user clarity, please update the documentation to explain this fallback mechanism.
| it('token without chain should return error', async () => { | ||
| const result = await parsePaymentURL(['0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', 'ETH']) | ||
| expect(result.error?.message).toEqual(EParseUrlError.INVALID_CHAIN) | ||
| expect(result.parsedUrl).toBeNull() | ||
| expect(result.error).toBeNull() | ||
| expect(result.parsedUrl).toEqual( | ||
| expect.objectContaining({ | ||
| recipient: { | ||
| identifier: '0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', | ||
| recipientType: 'ADDRESS', | ||
| resolvedAddress: '0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', | ||
| }, | ||
| amount: undefined, | ||
| token: expect.objectContaining({ | ||
| symbol: 'ETH', | ||
| }), | ||
| chain: expect.objectContaining({ | ||
| chainId: 42161, | ||
| }), | ||
| }) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Test name contradicts actual implementation
The test name "token without chain should return error" contradicts the actual assertions in the test, which now expect Arbitrum to be used as the default chain (not an error). This mismatch between the test name and implementation can cause confusion for developers trying to understand the expected behavior.
Update the test name to match the current implementation:
-it('token without chain should return error', async () => {
+it('should default to Arbitrum chain when only token symbol is provided', async () => {📝 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.
| it('token without chain should return error', async () => { | |
| const result = await parsePaymentURL(['0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', 'ETH']) | |
| expect(result.error?.message).toEqual(EParseUrlError.INVALID_CHAIN) | |
| expect(result.parsedUrl).toBeNull() | |
| expect(result.error).toBeNull() | |
| expect(result.parsedUrl).toEqual( | |
| expect.objectContaining({ | |
| recipient: { | |
| identifier: '0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', | |
| recipientType: 'ADDRESS', | |
| resolvedAddress: '0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', | |
| }, | |
| amount: undefined, | |
| token: expect.objectContaining({ | |
| symbol: 'ETH', | |
| }), | |
| chain: expect.objectContaining({ | |
| chainId: 42161, | |
| }), | |
| }) | |
| ) | |
| }) | |
| it('should default to Arbitrum chain when only token symbol is provided', async () => { | |
| const result = await parsePaymentURL(['0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', 'ETH']) | |
| expect(result.error).toBeNull() | |
| expect(result.parsedUrl).toEqual( | |
| expect.objectContaining({ | |
| recipient: { | |
| identifier: '0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', | |
| recipientType: 'ADDRESS', | |
| resolvedAddress: '0x0fdaEB9903A291aB8450DFA25B3fa962E075547A', | |
| }, | |
| amount: undefined, | |
| token: expect.objectContaining({ | |
| symbol: 'ETH', | |
| }), | |
| chain: expect.objectContaining({ | |
| chainId: 42161, | |
| }), | |
| }) | |
| ) | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Payment/PaymentForm/index.tsx (3)
195-232: Implemented token price fetching.This new effect properly fetches token prices and calculates USD values, addressing TASK-9434 (correctly displaying USD values). The implementation includes proper loading state management and error handling.
Consider enhancing the error logging on line 225 to include more context about the specific error, which would help with debugging:
-console.error('Error fetching token price:', error) +console.error('Error fetching token price for token address:', requestDetails.tokenAddress, 'on chain:', requestDetails.chainId, error)
368-376: Enhanced payment details display with USD values.The implementation now correctly displays both USD and token values, addressing TASK-9314 (displaying numerical values with two significant digits).
There's a typo in the variable name
dispalyAmount(should bedisplayAmount):-const dispalyAmount = tokenUsdValue +const displayAmount = tokenUsdValue ? `$${tokenUsdValue} ( ${formatAmount(tokenAmount)} ${tokenSymbol} )` : `${formatAmount(tokenAmount)} ${tokenSymbol}` - <PaymentInfoRow label="Amount" value={dispalyAmount} /> + <PaymentInfoRow label="Amount" value={displayAmount} />
284-284: Hardcoded currency to USD.While this works for the current requirements, hardcoding 'USD' as the currency might limit future flexibility if other currencies need to be supported.
Consider making the currency configurable or derived from the application state:
- currency: 'USD', + currency: requestDetails?.currency || 'USD',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Global/TokenSelector/TokenSelector.tsx(0 hunks)src/components/Payment/PaymentForm/index.tsx(9 hunks)src/components/Payment/Views/Confirm.payment.view.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- src/components/Global/TokenSelector/TokenSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Payment/Views/Confirm.payment.view.tsx
🧰 Additional context used
🧠 Learnings (1)
src/components/Payment/PaymentForm/index.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (7)
src/components/Payment/PaymentForm/index.tsx (7)
22-30: Improved imports with token price fetching functionality.The imports have been properly updated to include the necessary functions for token price fetching and amount formatting, which aligns with the PR objectives.
50-52: Added state variables for USD conversion.These new state variables provide the foundation for addressing TASK-9434 (correctly displaying USD values) by tracking the token price, USD value, and loading state.
108-108: Fixed token symbol fallback.Returning 'USDC' as a default token symbol addresses TASK-9454 (resolving issues related to reading the token symbol) by providing a sensible fallback value.
131-142: Improved token decimals handling logic.The enhanced token decimals handling addresses TASK-9455 (fixing fractional decimals error) by implementing a more robust fallback mechanism, checking multiple sources for decimals information before defaulting to 6.
164-170: Enhanced chain token selection with defaults.Setting a default token (USDC) when none is specified improves user experience and aligns with TASK-9467 (setting default chain to Arbitrum), though this implementation focuses on token defaults rather than chain defaults.
175-177: Added proper token decimals handling.This change ensures token decimals are properly set when available in the token object, addressing TASK-9455 (fixing fractional decimals error).
267-277: Added USD-to-token conversion logic.The conversion logic correctly handles switching between USD and token denominations based on user input.
Be cautious with division by
selectedTokenPriceif the token price is very small (close to zero), as this could result in extremely large token amounts. Consider adding a check to ensure the token price is above a minimum threshold.+if (!selectedTokenPrice || selectedTokenPrice < 0.000001) { -if (!selectedTokenPrice) { throw new Error('Token price not available') }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Payment/Views/Confirm.payment.view.tsx (2)
145-173: Improved error handling for cross-chain transactions.The addition of a try-catch block with more detailed error logging will make debugging easier. Good use of structured error handling to improve system robustness.
However, I notice that when you throw errors in the catch block, you're not preserving the original error's stack trace, which could be valuable for debugging.
- } catch (error) { - console.error('Cross-chain tx preparation failed:', error) - throw new Error('Failed to prepare cross-chain transaction') + } catch (error) { + console.error('Cross-chain tx preparation failed:', error) + throw new Error(`Failed to prepare cross-chain transaction: ${error instanceof Error ? error.message : String(error)}`) }
443-444: Improved recipient display logic.Good enhancement to conditionally display recipient information based on type, using address shortening when appropriate. This improves readability for users.
Consider adding a fallback for potential null/undefined values to make the code more robust.
- label={`${parsedPaymentData?.recipient.recipientType === 'ADDRESS' ? shortenAddressLong(parsedPaymentData?.recipient.identifier) : parsedPaymentData?.recipient.identifier} will receive`} + label={`${parsedPaymentData?.recipient?.recipientType === 'ADDRESS' ? shortenAddressLong(parsedPaymentData?.recipient?.identifier || '') : parsedPaymentData?.recipient?.identifier || 'Recipient'} will receive`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Payment/Views/Confirm.payment.view.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (4)
src/components/Payment/Views/Confirm.payment.view.tsx (4)
16-22: Import changes look good.The replacement of
formatTokenAmountwithformatAmountand the addition ofshortenAddressLonghelps address TASK-9314 for better display of numerical values and TASK-9454 for improved token symbol display.
85-86: Good fix for fractional decimals display.This change properly formats token amounts with the correct number of decimals, addressing TASK-9455 (fractional decimals error).
91-91: Correctly using formatted amount.Using the properly formatted token amount ensures consistent decimal handling across the application.
438-438: Correctly using formatAmount.Nice replacement of the formatting function to ensure consistent display of token amounts with the proper number of decimal places, addressing TASK-9314.
| case 'ADDRESS': | ||
| let tokenSymbol = selectedTokenData?.symbol ?? getTokenSymbol(recipientTokenAddress, recipientChainId) | ||
| if (!tokenSymbol) { | ||
| throw new Error('Failed to get token symbol') |
There was a problem hiding this comment.
question(blocking): why? in this case we might have a tokenAddress a token and inform USDC when it's not
There was a problem hiding this comment.
this was leading to error when going directly to p.me/kushagrasarathe.eth if token was not defined in url, i added usdc as default to avoid this error, and users now have option to select token/chain in ui
| } | ||
| return 6 | ||
| default: | ||
| throw new Error('Invalid recipient type') |
There was a problem hiding this comment.
leading to bad ux, token decimals were not being properly found and set, ideally instead of throwing errors for these scenarios, i'd prefer showing the ui and let users select token/chain, thats why this solution
| }, [isPeanutWallet]) | ||
|
|
||
| // fetche token price | ||
| useEffect(() => { |
There was a problem hiding this comment.
question: why is this needed and why the current fetching in the tokenselectorcontext is not working?
also typo in comment
There was a problem hiding this comment.
@jjramirezn the token selector context works when a token/chain is selected in it, but for cases like when peanut wallet is selected or on the first load, the requested token price aren't fetched, and also the price would switch if i switch token in the selector, so to specifically fetch token price details of the requested tokens, added this call
There was a problem hiding this comment.
this is not used anywhere now, also slice in redux
| }, | ||
| address | ||
| ) | ||
| try { |
There was a problem hiding this comment.
noted, missed it, was trying somethings, removing it, thanks
| if (!chainDetails) { | ||
| return { parsedUrl: null, error: { message: EParseUrlError.INVALID_CHAIN, recipient } } | ||
| if (!chainDetails && !isPeanutRecipient) { | ||
| // default to arb even for non-USERNAME recipients if no chain is specified |
There was a problem hiding this comment.
dont know if this behavior is desired, I think in this case we should be explicit and have chain
There was a problem hiding this comment.
defaulted it to arb, to avoid client side errors, users anyways have the option to change it from token selector
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Global/TokenSelector/TokenSelector.tsx(2 hunks)src/components/Payment/PaymentForm/index.tsx(9 hunks)src/components/Payment/Views/Confirm.payment.view.tsx(4 hunks)src/utils/__tests__/url-parser.test.ts(3 hunks)src/utils/general.utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Global/TokenSelector/TokenSelector.tsx
- src/components/Payment/Views/Confirm.payment.view.tsx
🧰 Additional context used
🧠 Learnings (1)
src/components/Payment/PaymentForm/index.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#413
File: src/context/tokenSelector.context.tsx:118-123
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (10)
src/utils/__tests__/url-parser.test.ts (3)
209-228: Test name should match the new behaviorThe test name "token without chain should return arbitrum as default chain" correctly describes the expected behavior, but is inconsistent with the previously reviewed test name in the exact same location. This appears to be a renamed test to match the new implementation where Arbitrum is used as the default chain instead of returning an error.
237-256: Test name updated to match implementationThe test name now accurately describes the behavior of defaulting to Arbitrum when only a token symbol is provided, which aligns with the PR objective (TASK-9467) to set Arbitrum as the default chain when a token is specified but the chain is not included in the URL.
364-381: Consistent implementation of default chain behaviorThis test case now properly verifies that providing a token without a chain defaults to Arbitrum, which is consistent with the other test cases and the PR objectives.
src/utils/general.utils.ts (1)
893-900: Improved error handling for token symbol resolutionThe function signature and implementation have been updated to handle undefined inputs gracefully, preventing potential runtime errors when token address or chain ID is undefined. This change directly addresses TASK-9454 (issue related to reading the token symbol).
Key improvements:
- Function now accepts
undefinedparameters- Early return if required parameters are missing
- Proper null checking before accessing chain tokens
src/components/Payment/PaymentForm/index.tsx (6)
50-52: Added state for tracking token price and USD valuesNew state variables to support displaying USD values (TASK-9434) and manage loading states during token price fetching. This is a good addition for improving user experience and tracking related data.
108-109: Default to USDC when token symbol can't be determinedInstead of throwing an error, the code now defaults to 'USDC' when a token symbol cannot be determined. This addresses TASK-9535 which requires defaulting to USDC token selection when only chain and recipient are specified.
131-141: Improved token decimals resolution with proper fallbacksThe code now handles token decimals resolution more gracefully by:
- First checking if decimals are available in the selected token data
- Then trying to fetch decimals from token address and chain ID
- Defaulting to 6 decimals (standard for most stablecoins) if all else fails
This helps address TASK-9455 regarding fixing errors with fractional decimals.
164-170: Default to USDC token when not specifiedWhen a chain is provided but no token is specified, the code now defaults to USDC on that chain, aligning with TASK-9535. This provides a better user experience by ensuring there's always a valid token selected.
368-377: Improved display of requested payment details with USD valueThe code now shows both the token amount and its USD equivalent (when available), which improves transparency for users and satisfies TASK-9434 (correctly displaying USD values) and TASK-9314 (displaying numerical values with appropriate decimal precision).
265-277: Proper calculation of token amount based on denominationThe logic now properly handles both token and USD denominations, calculating the appropriate conversion when needed. This ensures that regardless of how the user enters the amount, the correct token amount is used for the transaction.
| useEffect(() => { | ||
| if (!requestDetails?.tokenAddress || !requestDetails?.chainId || !requestDetails?.tokenAmount) return | ||
|
|
||
| const getTokenPriceData = async () => { | ||
| setIsFetchingTokenPrice(true) | ||
| try { | ||
| const priceData = await fetchTokenPrice(requestDetails.tokenAddress, requestDetails.chainId) | ||
|
|
||
| if (priceData) { | ||
| setRequestedTokenPrice(priceData.price) | ||
|
|
||
| // calculate USD value | ||
| const tokenAmount = parseFloat(requestDetails.tokenAmount) | ||
| const usdValue = formatAmount(tokenAmount * priceData.price) | ||
|
|
||
| dispatch( | ||
| paymentActions.setPaymentValues({ | ||
| tokenValue: requestDetails.tokenAmount, | ||
| usdValue, | ||
| }) | ||
| ) | ||
|
|
||
| setInputDenomination('USD') | ||
| setInputTokenAmount(usdValue) | ||
| setUsdValue(usdValue) | ||
| } else { | ||
| console.log('Failed to fetch token price data') | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching token price:', error) | ||
| } finally { | ||
| setIsFetchingTokenPrice(false) | ||
| } | ||
| } | ||
|
|
||
| getTokenPriceData() | ||
| }, [requestDetails]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Token price fetching implementation needs dependency array correction
The implemented token price fetching feature addresses TASK-9434 (correctly displaying USD values), but there's an issue with the dependency array in the useEffect hook.
The useEffect uses dispatch inside its callback but doesn't include it in the dependency array. This could lead to stale closures if the dispatch function changes. Update the dependency array to include all dependencies:
- }, [requestDetails])
+ }, [requestDetails, dispatch])📝 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.
| useEffect(() => { | |
| if (!requestDetails?.tokenAddress || !requestDetails?.chainId || !requestDetails?.tokenAmount) return | |
| const getTokenPriceData = async () => { | |
| setIsFetchingTokenPrice(true) | |
| try { | |
| const priceData = await fetchTokenPrice(requestDetails.tokenAddress, requestDetails.chainId) | |
| if (priceData) { | |
| setRequestedTokenPrice(priceData.price) | |
| // calculate USD value | |
| const tokenAmount = parseFloat(requestDetails.tokenAmount) | |
| const usdValue = formatAmount(tokenAmount * priceData.price) | |
| dispatch( | |
| paymentActions.setPaymentValues({ | |
| tokenValue: requestDetails.tokenAmount, | |
| usdValue, | |
| }) | |
| ) | |
| setInputDenomination('USD') | |
| setInputTokenAmount(usdValue) | |
| setUsdValue(usdValue) | |
| } else { | |
| console.log('Failed to fetch token price data') | |
| } | |
| } catch (error) { | |
| console.error('Error fetching token price:', error) | |
| } finally { | |
| setIsFetchingTokenPrice(false) | |
| } | |
| } | |
| getTokenPriceData() | |
| }, [requestDetails]) | |
| useEffect(() => { | |
| if (!requestDetails?.tokenAddress || !requestDetails?.chainId || !requestDetails?.tokenAmount) return | |
| const getTokenPriceData = async () => { | |
| setIsFetchingTokenPrice(true) | |
| try { | |
| const priceData = await fetchTokenPrice(requestDetails.tokenAddress, requestDetails.chainId) | |
| if (priceData) { | |
| setRequestedTokenPrice(priceData.price) | |
| // calculate USD value | |
| const tokenAmount = parseFloat(requestDetails.tokenAmount) | |
| const usdValue = formatAmount(tokenAmount * priceData.price) | |
| dispatch( | |
| paymentActions.setPaymentValues({ | |
| tokenValue: requestDetails.tokenAmount, | |
| usdValue, | |
| }) | |
| ) | |
| setInputDenomination('USD') | |
| setInputTokenAmount(usdValue) | |
| setUsdValue(usdValue) | |
| } else { | |
| console.log('Failed to fetch token price data') | |
| } | |
| } catch (error) { | |
| console.error('Error fetching token price:', error) | |
| } finally { | |
| setIsFetchingTokenPrice(false) | |
| } | |
| } | |
| getTokenPriceData() | |
| }, [requestDetails, dispatch]) |
| }, [isPeanutWallet]) | ||
|
|
||
| // fetche token price | ||
| useEffect(() => { |
There was a problem hiding this comment.
this is not used anywhere now, also slice in redux
Summary by CodeRabbit
New Features
Bug Fixes