[TASK-6946]: fix fee calculation in cashout and request flows #594
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces significant modifications to various components, primarily enhancing transaction fee and slippage calculations. A new Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
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: 3
🧹 Nitpick comments (4)
src/components/Request/Pay/Views/Initial.view.tsx (4)
99-106: Initialize range states carefully to avoid confusion later
Here, you set default values forfeeRangeandslippageRangeusinguseState. While the approach works, be mindful that these range objects store string values (formatted numbers). If you need to do further numeric calculations on them, consider storing numeric values first and formatting only before display. This helps avoid unexpected type conversions down the line.
107-115: Ensure consistent calculation for totalAmountRange initialization
Using the same value for bothminandmaxis fine as a default. However, if the application later needs to reflect additional fees or slippage right away, consider calculating at least a minimal buffer for themaxfield to better reflect a practical upper bound from the start.
480-515: Leverage InfoRow effectively but confirm placeholders
UsingInfoRowto display fee, slippage, and total is a neat approach. However, watch out for scenarios where values might briefly be"0"during calculation or after resets. If users see"0-0"flicker, it might cause confusion. Optionally, show a loading indicator or default placeholders while recalculating.
572-614: Consider storing numeric values in InfoRow for clarity
Currently,valueMinandvalueMaxare strings concatenated with “$” inside the component. If you intend to handle multiple currency symbols or do further numeric manipulation in the future, you might benefit from passing numeric props toInfoRowand handling formatting here. For now, it works fine as is, but it’s something to keep in mind for future expansions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Request/Pay/Views/Initial.view.tsx(11 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (2)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
|
|
||
| // calculate fee range (assuming 5% variation) | ||
| const minFee = Number(feeEstimation) | ||
| const maxFee = minFee * 1.05 | ||
| setFeeRange({ | ||
| min: minFee.toFixed(2), | ||
| max: maxFee.toFixed(2), | ||
| }) | ||
|
|
||
| // calculate slippage range | ||
| const minSlippageAmount = | ||
| ((slippagePercentage * 0.5) / 100) * selectedTokenData!.price * Number(estimatedFromAmount) | ||
| const maxSlippageAmount = | ||
| (slippagePercentage / 100) * selectedTokenData!.price * Number(estimatedFromAmount) | ||
| setSlippageRange({ | ||
| min: minSlippageAmount.toFixed(2), | ||
| max: maxSlippageAmount.toFixed(2), | ||
| }) | ||
|
|
||
| const minTotal = estimatedFromAmount + Number(feeRange.min) + Number(slippageRange.min) | ||
| const maxTotal = estimatedFromAmount + Number(feeRange.max) + Number(slippageRange.max) | ||
| setTotalAmountRange({ | ||
| min: formatAmount(minTotal), | ||
| max: formatAmount(maxTotal), | ||
| }) | ||
|
|
There was a problem hiding this comment.
Apply the same fix for xChain concurrency
Same concern when handling X-chain fees and slippage: you’re calling setFeeRange and setSlippageRange, then immediately referencing feeRange.min / max and slippageRange.min / max. Store your newly computed fees and slippage in local variables and then recompute totals to avoid reading stale state.
There was a problem hiding this comment.
pls always resolve coderabbit comments - mostly they're unnecessary nitpicks, but sometimes they're useful
There was a problem hiding this comment.
@Hugo0, noted! We will keep resolving CodeRabbit’s comments to ensure we don’t miss any useful suggestions.
✏️ Learnings added
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.487Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| setSlippageRange({ min: '0', max: '0' }) // reset slippage | ||
| // recalculate maxTotal with just the requested amount and gas | ||
| const requestedAmountInUsd = tokenPriceData | ||
| ? Number(requestLinkData.tokenAmount) * tokenPriceData.price | ||
| : Number(requestLinkData.tokenAmount) | ||
| const minTotal = requestedAmountInUsd + Number(feeRange.min) + Number(slippageRange.min) | ||
| const maxTotal = requestedAmountInUsd + Number(feeRange.max) + Number(slippageRange.max) | ||
| setTotalAmountRange({ | ||
| min: formatAmount(minTotal), | ||
| max: formatAmount(maxTotal), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resetting token and chain might need local variable usage
Similar concurrency issue: after resetting feeRange or slippageRange, you compute minTotal and maxTotal using feeRange.min and slippageRange.min. Either store them in local variables upfront or compute inside the same state setter callback.
| // for non-xchain, fee is just the gas cost | ||
| const gasEstimate = estimatedGasCost ?? 0 | ||
| setFeeRange({ | ||
| min: formatTokenAmount(gasEstimate, 3) ?? '0', | ||
| max: formatTokenAmount(gasEstimate * 1.1, 3) ?? '0', // adding 10% buffer | ||
| }) | ||
|
|
||
| // calculate total with requested amount and gas | ||
| const requestedAmountInUsd = tokenPriceData | ||
| ? Number(requestLinkData.tokenAmount) * tokenPriceData.price | ||
| : Number(requestLinkData.tokenAmount) | ||
| const minTotal = requestedAmountInUsd + Number(feeRange.min) + Number(slippageRange.min) | ||
| const maxTotal = requestedAmountInUsd + Number(feeRange.max) + Number(slippageRange.max) | ||
| setTotalAmountRange({ | ||
| min: formatAmount(minTotal), | ||
| max: formatAmount(maxTotal), | ||
| }) | ||
|
|
There was a problem hiding this comment.
Avoid relying on outdated state when calculating totals
In React, calls to setFeeRange do not immediately update feeRange. If you then call Number(feeRange.min) or Number(feeRange.max) in the same function call stack, you may still get the old state. To prevent a potential race condition, store the new fee values in local variables and then use those values to compute the total before calling setTotalAmountRange.
- setFeeRange({
- min: formatTokenAmount(gasEstimate, 3) ?? '0',
- max: formatTokenAmount(gasEstimate * 1.1, 3) ?? '0',
- })
...
- const minTotal = requestedAmountInUsd + Number(feeRange.min) + Number(slippageRange.min)
- const maxTotal = requestedAmountInUsd + Number(feeRange.max) + Number(slippageRange.max)
+ const newFeeMin = formatTokenAmount(gasEstimate, 3) ?? '0'
+ const newFeeMax = formatTokenAmount(gasEstimate * 1.1, 3) ?? '0'
+ setFeeRange({ min: newFeeMin, max: newFeeMax })
+ const minTotal = requestedAmountInUsd + Number(newFeeMin) + Number(slippageRange.min)
+ const maxTotal = requestedAmountInUsd + Number(newFeeMax) + Number(slippageRange.max)
setTotalAmountRange({
min: formatAmount(minTotal),
max: formatAmount(maxTotal),
})📝 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.
| // for non-xchain, fee is just the gas cost | |
| const gasEstimate = estimatedGasCost ?? 0 | |
| setFeeRange({ | |
| min: formatTokenAmount(gasEstimate, 3) ?? '0', | |
| max: formatTokenAmount(gasEstimate * 1.1, 3) ?? '0', // adding 10% buffer | |
| }) | |
| // calculate total with requested amount and gas | |
| const requestedAmountInUsd = tokenPriceData | |
| ? Number(requestLinkData.tokenAmount) * tokenPriceData.price | |
| : Number(requestLinkData.tokenAmount) | |
| const minTotal = requestedAmountInUsd + Number(feeRange.min) + Number(slippageRange.min) | |
| const maxTotal = requestedAmountInUsd + Number(feeRange.max) + Number(slippageRange.max) | |
| setTotalAmountRange({ | |
| min: formatAmount(minTotal), | |
| max: formatAmount(maxTotal), | |
| }) | |
| // for non-xchain, fee is just the gas cost | |
| const gasEstimate = estimatedGasCost ?? 0 | |
| const newFeeMin = formatTokenAmount(gasEstimate, 3) ?? '0' | |
| const newFeeMax = formatTokenAmount(gasEstimate * 1.1, 3) ?? '0' | |
| setFeeRange({ min: newFeeMin, max: newFeeMax }) | |
| // calculate total with requested amount and gas | |
| const requestedAmountInUsd = tokenPriceData | |
| ? Number(requestLinkData.tokenAmount) * tokenPriceData.price | |
| : Number(requestLinkData.tokenAmount) | |
| const minTotal = requestedAmountInUsd + Number(newFeeMin) + Number(slippageRange.min) | |
| const maxTotal = requestedAmountInUsd + Number(newFeeMax) + Number(slippageRange.max) | |
| setTotalAmountRange({ | |
| min: formatAmount(minTotal), | |
| max: formatAmount(maxTotal), | |
| }) |
| min: formatTokenAmount(gasEstimate, 3) ?? '0', | ||
| max: formatTokenAmount(gasEstimate * 1.1, 3) ?? '0', |
There was a problem hiding this comment.
Q & issues, blocking:
- Where do these super arbitrary values come from?
- If we're going to have gas consts multipliers like this, then these should probably not be defined inline in this page
- we shouldn't have custom code for fees in request pay, and not have this in claim create, claim, and cashout, and request create. These all should share a common fees section!
- where does estimatedGasCost come from? SDK? In that case, it already has a buffer included! Also, the actual gas cost will be below the estimatedGasCost, sometimes as much as 50%
|
|
||
| // calculate fee range (assuming 5% variation) | ||
| const minFee = Number(feeEstimation) | ||
| const maxFee = minFee * 1.05 | ||
| setFeeRange({ | ||
| min: minFee.toFixed(2), | ||
| max: maxFee.toFixed(2), | ||
| }) | ||
|
|
||
| // calculate slippage range | ||
| const minSlippageAmount = | ||
| ((slippagePercentage * 0.5) / 100) * selectedTokenData!.price * Number(estimatedFromAmount) | ||
| const maxSlippageAmount = | ||
| (slippagePercentage / 100) * selectedTokenData!.price * Number(estimatedFromAmount) | ||
| setSlippageRange({ | ||
| min: minSlippageAmount.toFixed(2), | ||
| max: maxSlippageAmount.toFixed(2), | ||
| }) | ||
|
|
||
| const minTotal = estimatedFromAmount + Number(feeRange.min) + Number(slippageRange.min) | ||
| const maxTotal = estimatedFromAmount + Number(feeRange.max) + Number(slippageRange.max) | ||
| setTotalAmountRange({ | ||
| min: formatAmount(minTotal), | ||
| max: formatAmount(maxTotal), | ||
| }) | ||
|
|
There was a problem hiding this comment.
pls always resolve coderabbit comments - mostly they're unnecessary nitpicks, but sometimes they're useful
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Global/InfoRow/index.tsx (1)
1-26: Consider making themoreInfoTextprop optional.
The current interface always requires amoreInfoTexteven if no additional explanation is needed. Making this prop optional (and conditionally rendering theMoreInfocomponent) could help keep the interface flexible.src/components/Global/FeeDescription/index.tsx (1)
1-75: Looks good overall. Consider adding unit tests for expand/collapse behavior.
The internal state toggles the collapsible section, which is crucial to the user experience. A unit test or two verifying this functionality would ensure it remains stable in future refactors.src/components/Request/Pay/Views/Initial.view.tsx (2)
104-145: Separate out constants for readability.
The numeric values (like0.7for network fees and0.1for slippage) seem arbitrary. Consider defining them in a constants file or top-level constants for better maintainability and clarity.- expected: isXChain ? Number(txFee) * 0.7 : Number(estimatedGasCost) * 0.7, - ... - expected: Number(calculatedSlippage) * 0.1, + const NETWORK_FEE_PERCENT = 0.7 + const SLIPPAGE_EXPECTED_FACTOR = 0.1 + ... + expected: isXChain ? Number(txFee) * NETWORK_FEE_PERCENT + : Number(estimatedGasCost) * NETWORK_FEE_PERCENT, + ... + expected: Number(calculatedSlippage) * SLIPPAGE_EXPECTED_FACTOR,
398-402: Improve accessibility by providing unique alt text.
Currently, both images share the samealtvalue ("logo"). Use a descriptive alt text for each image for better accessibility.<img src={tokenRequestedLogoURI ?? ''} - alt="logo" + alt={tokenRequestedSymbol ? `${tokenRequestedSymbol} logo` : 'token logo'} /> <img src={...} - alt="logo" + alt="chain icon" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
src/components/Global/FeeDescription/index.tsx(1 hunks)src/components/Global/InfoRow/index.tsx(1 hunks)src/components/Request/Pay/Views/Initial.view.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (2)
1-1: No issues with this new import.
This line simply adds the hook for creating links. Looks acceptable.
433-455: Implementation aligns with the new FeeDescription and InfoRow components.
The structured breakdown of fees and maximum payment looks coherent. Good job using the new components for a clean, user-friendly layout.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
Line range hint
332-397: Potential race condition in fee estimation effect.The effect depends on multiple state variables and makes async calls. Consider using a cleanup function to prevent stale updates and race conditions.
useEffect(() => { + let isMounted = true; const estimateTxFee = async () => { if (!isXChain) { clearError() setViewState(ViewState.READY_TO_PAY) return } try { clearError() setLoadingState('Preparing transaction') const txData = await createXChainUnsignedTx({ tokenData: selectedTokenData!, requestLink: requestLinkData, senderAddress: address!, }) + if (!isMounted) return; const { feeEstimation, estimatedFromAmount, slippagePercentage, unsignedTxs: _xChainUnsignedTxs, } = txData setEstimatedFromValue(estimatedFromAmount) setSlippagePercentage(slippagePercentage) setXChainUnsignedTxs(_xChainUnsignedTxs) clearError() setTxFee(Number(feeEstimation).toFixed(2)) setViewState(ViewState.READY_TO_PAY) } catch (error) { + if (!isMounted) return; setErrorState({ showError: true, errorMessage: ERR_NO_ROUTE }) setIsFeeEstimationError(true) setSlippagePercentage(undefined) setXChainUnsignedTxs(undefined) setTxFee('0') } } if (!isConnected || !address) { setViewState(ViewState.INITIAL) return } if (isXChain && !selectedTokenData) { if (!isFetchingTokenData) { setErrorState({ showError: true, errorMessage: ERR_NO_ROUTE }) setIsFeeEstimationError(true) setTxFee('0') } return } estimateTxFee() + return () => { + isMounted = false; + } }, [isConnected, address, selectedTokenData, requestLinkData, isXChain, isFetchingTokenData])
🧹 Nitpick comments (4)
src/components/Create/Link/Confirm.view.tsx (1)
296-311: Conditional fee display logic
Switching toInfoRowclarifies the fee presentation. A small nitpick: ensure the$0.01threshold is widely understood across the codebase or configured in a shared utility for consistent formatting.src/components/Global/InfoRow/index.tsx (1)
12-29: Core InfoRow component
Implementation is concise and covers typical use cases (e.g., loading vs. showing content). Consider adding unit tests to ensure the component behaves consistently across states.src/components/Global/FeeDescription/index.tsx (1)
60-76: Detailed InfoRow usage
Presenting separate rows for network cost and slippage is effective. For complex fees or token conversions, ensure different edge cases (e.g., zero network fee or negative slippage) are handled or validated at a higher level.src/components/Request/Pay/Views/Initial.view.tsx (1)
398-402: Add alt text for better accessibility.The image alt text should be more descriptive for better accessibility.
<img src={tokenRequestedLogoURI ?? ''} className="absolute left-0 top-0 h-6 w-6" - alt="logo" + alt={`${tokenRequestedSymbol} token logo`} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/assets/animations(1 hunks)src/components/Claim/Link/Initial.view.tsx(2 hunks)src/components/Claim/Link/Onchain/Confirm.view.tsx(2 hunks)src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Global/FeeDescription/index.tsx(1 hunks)src/components/Global/InfoRow/index.tsx(1 hunks)src/components/Request/Pay/Views/Initial.view.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/assets/animations
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (15)
src/components/Claim/Link/Initial.view.tsx (5)
2-3: Imports for CrispButton and AddressLink look good
These additions cleanly introduce the Crisp chat button and address link features.
6-6: New import for InfoRow component
Importing theInfoRowcomponent is a neat way to organize and present fee or summary information in a structured format.
16-16: Introducing ActionType and estimatePoints
This import is beneficial for handling points estimation logic in a centralized utility.
22-29: Utility imports from '@/utils'
Great to seeErrorHandler,formatTokenAmount, etc., consolidated in a utility module. This fosters reusability and keeps the main component clean.
512-518: Validate the hardcoded fee and loading flag
Using anInfoRowwithvalue={'$ 0'}might confuse users if certain cross-chain scenarios incur hidden costs. Consider verifying whether$0is always correct or if dynamic values are needed.src/components/Claim/Link/Onchain/Confirm.view.tsx (2)
3-3: Reorganized imports for new components
The rearrangement and inclusion ofIcon,InfoRow, and utility modules are consistent with the adoption of structured components.Also applies to: 6-6, 10-10, 12-12
212-217: Review the “$ 0” fees display
Displaying$ 0may be misleading if actual fees apply in certain edge cases. Validate the scenario before finalizing.src/components/Create/Link/Confirm.view.tsx (1)
2-2: New imports for icons and InfoRow
Import statements forIcon,InfoRow, and utility functions are introduced cleanly; everything reflects a well-structured approach.Also applies to: 4-5, 6-7, 10-11, 14-16, 24-25
src/components/Global/InfoRow/index.tsx (2)
1-2: Icon and MoreInfo imports
Good to see these essential visual components brought in, keeping the InfoRow interface concise yet functional.
4-10: Clear and minimal InfoRowProps interface
This interface is straightforward, capturing the label, value, icon name, and optional loading. This fosters clarity.src/components/Global/FeeDescription/index.tsx (4)
1-4: Imports for Icon, formatAmount, and InfoRow
These provide the foundation for a well-rounded, self-contained fee breakdown.
6-8: INITIAL_STATE for collapsing
Using a local state object withisExpandedto control expansion is a clear approach for a small toggle.
15-20: FeeDescriptionProps interface
Well-defined, covering all necessary fields: estimatedFee, networkFee, optional slippage, and loading. Good job ensuring typed data integrity.
22-50: Toggling header logic
The toggle approach for displaying details is user-friendly, and the icon rotation nicely indicates expansion. Code is straightforward and maintainable.src/components/Request/Pay/Views/Initial.view.tsx (1)
433-457: Clean implementation of fee description UI components!The implementation properly handles loading states, error conditions, and provides clear user feedback through the InfoRow component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (2)
104-152: Extract magic numbers and utility functionsWhile the calculations are well-documented, there are a few improvements that could enhance maintainability:
- Move the magic numbers to named constants at the file or module level
- Extract the formatNumberSafely utility function for reuse
- Add TypeScript types for the calculation results
+ const NETWORK_FEE_EXPECTED_MULTIPLIER = 0.7 // 70% of maximum network fee + const SLIPPAGE_EXPECTED_MULTIPLIER = 0.1 // 10% of maximum slippage + + interface FeeCalculationResult { + networkFee: { expected: string } + slippage?: { expected: string; max: string } + expectedTotal: string + totalMax: string + } + + const formatNumberSafely = (num: number): string => { + return formatAmount(num < 0.01 && num > 0 ? 0.01 : num) + } const feeCalculations = useMemo(() => { - // percentage of maximum network fee to use as expected fee (70%) - const EXPECTED_NETWORK_FEE_MULTIPLIER = 0.7 - - // percentage of maximum slippage to use as expected slippage (10%) - const EXPECTED_SLIPPAGE_MULTIPLIER = 0.1 // gas/network cost calculation (70% of max for expected) const networkFee = { expected: isXChain - ? Number(txFee) * EXPECTED_NETWORK_FEE_MULTIPLIER - : Number(estimatedGasCost) * EXPECTED_NETWORK_FEE_MULTIPLIER, + ? Number(txFee) * NETWORK_FEE_EXPECTED_MULTIPLIER + : Number(estimatedGasCost) * NETWORK_FEE_EXPECTED_MULTIPLIER, max: isXChain ? Number(txFee) : Number(estimatedGasCost), } // slippage calculation (10% of max for expected) const slippage = calculatedSlippage ? { - expected: Number(calculatedSlippage) * EXPECTED_SLIPPAGE_MULTIPLIER, + expected: Number(calculatedSlippage) * SLIPPAGE_EXPECTED_MULTIPLIER, max: Number(calculatedSlippage), } : undefined // ... rest of the calculations ... - const formatNumberSafely = (num: number) => { - return formatAmount(num < 0.01 && num > 0 ? 0.01 : num) - } - return { + return { networkFee: { expected: formatNumberSafely(networkFee.expected || 0), }, slippage: slippage ? { expected: formatNumberSafely(slippage.expected || 0), max: formatNumberSafely(slippage.max || 0), } : undefined, expectedTotal: formatNumberSafely(networkFee.expected + (slippage?.expected || 0) || 0), totalMax: formatNumberSafely(totalMax || 0), - } + } as FeeCalculationResult }, [isXChain, txFee, estimatedGasCost, calculatedSlippage, tokenPriceData, requestLinkData.tokenAmount])
437-461: Consider using loading context to avoid prop drillingThe loading state is being passed down to both
FeeDescriptionandInfoRowcomponents. Consider using the existingloadingStateContextinstead of prop drilling.- <FeeDescription - loading={isLoading} + <FeeDescription estimatedFee={feeCalculations.expectedTotal} networkFee={feeCalculations.networkFee.expected} slippageRange={ feeCalculations.slippage ? { min: feeCalculations.slippage.expected, max: feeCalculations.slippage.max, } : undefined } /> - <InfoRow - loading={isLoading} + <InfoRow iconName="transfer" label="Total Max" value={`$ ${feeCalculations.totalMax}`} moreInfoText={ feeCalculations.slippage ? 'Maximum amount you will pay including requested amount, fees, and maximum slippage.' : 'Maximum amount you will pay including requested amount and network fees.' } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
1-24: LGTM! Clean and well-organized imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Global/FeeDescription/index.tsx (3)
10-13: Consider using TypeScript's built-in utility types for the range type.The
TRangetype could benefit from using TypeScript's built-in utility types for better type safety and reusability.-type TRange = { - min: number | string - max: number | string -} +type NumericValue = number | string +type TRange = Record<'min' | 'max', NumericValue>
22-27: Consider memoizing the toggle handler.The
handleExpandTogglefunction is recreated on every render. Consider memoizing it withuseCallback.-const handleExpandToggle = () => { +const handleExpandToggle = useCallback(() => { setToggleDetailedView((prev) => ({ isExpanded: !prev.isExpanded })) -} +}, [])
69-71: Consider extracting transition classes to constants.The transition classes could be extracted to constants for better maintainability and reuse.
+const TRANSITION_CLASSES = { + base: 'overflow-hidden transition-all duration-300 ease-in-out', + expanded: 'max-h-60 opacity-100', + collapsed: 'max-h-0 opacity-0' +} -className={`overflow-hidden transition-all duration-300 ease-in-out ${ - toggleDetailedView.isExpanded ? 'max-h-60 opacity-100' : 'max-h-0 opacity-0' -}`} +className={`${TRANSITION_CLASSES.base} ${ + toggleDetailedView.isExpanded ? TRANSITION_CLASSES.expanded : TRANSITION_CLASSES.collapsed +}`}src/components/Request/Pay/Views/Initial.view.tsx (1)
104-152: Consider extracting fee calculation logic to a custom hook.The fee calculation logic is complex and could benefit from being extracted into a custom hook for better reusability and testability.
// useFeeCalculations.ts export const useFeeCalculations = ({ isXChain, txFee, estimatedGasCost, calculatedSlippage, tokenPriceData, requestedTokenAmount }: FeeCalculationParams) => { return useMemo(() => { // ... existing calculation logic ... }, [isXChain, txFee, estimatedGasCost, calculatedSlippage, tokenPriceData, requestedTokenAmount]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Global/FeeDescription/index.tsx(1 hunks)src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (3)
src/components/Global/FeeDescription/index.tsx (2)
15-20: LGTM! Props interface is well-defined.The props interface clearly defines the expected types and optional properties.
29-41: LGTM! Early return pattern is well implemented.The early return pattern for the non-slippage case is clean and improves readability.
src/components/Request/Pay/Views/Initial.view.tsx (1)
437-465: LGTM! FeeDescription integration is well implemented.The FeeDescription component is properly integrated with appropriate loading states and conditional rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Cashout/Components/Initial.view.tsx (1)
130-146: Enhance error handling for gas fee estimation.While the error handling catches failures, it silently sets the gas cost to '0'. Consider notifying the user when gas estimation fails.
} catch (error) { console.error('Failed to estimate gas fee:', error) setEstimatedGasCost?.('0') + setErrorState({ + showError: true, + errorMessage: 'Failed to estimate gas fees. The displayed cost might be inaccurate.', + }) }src/components/Global/FeeDescription/index.tsx (1)
27-146: Consider enhancing accessibility.While the implementation is solid, consider these accessibility improvements:
- Add
aria-expandedattribute to the expandable section.- Add
role="button"andtabIndex={0}to the clickable div.- Add keyboard support for the expand/collapse action.
<div onClick={handleExpandToggle} + role="button" + tabIndex={0} + aria-expanded={toggleDetailedView.isExpanded} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + handleExpandToggle() + } + }} className="flex w-full cursor-pointer items-center justify-between gap-1 p-2 text-h8 text-gray-1 transition-colors duration-200 hover:bg-gray-50" >src/components/Offramp/Confirm.view.tsx (1)
643-678: Refactor fee calculation logic into a custom hook.The fee calculation logic in the useEffect is complex and could be reused elsewhere. Consider extracting it into a custom hook for better maintainability and reusability.
// src/hooks/useFeesCalculation.ts interface FeesCalculationParams { estimatedGasCost: string appliedPromoCode: string | null accountType: string crossChainDetails?: any offrampType: OfframpType usdValue?: string tokenPrice?: number claimLinkData?: any } interface FeesCalculationResult { totalFees: number amount: number hasError: boolean errorMessage?: string } export const useFeesCalculation = ({ estimatedGasCost, appliedPromoCode, accountType, crossChainDetails, offrampType, usdValue, tokenPrice, claimLinkData }: FeesCalculationParams): FeesCalculationResult => { const totalFees = appliedPromoCode ? parseFloat(estimatedGasCost ?? '0') : parseFloat(estimatedGasCost ?? '0') + (accountType === 'iban' ? 1 : 0.5) + (crossChainDetails ? parseFloat(estimatedGasCost ?? '0') * 0.1 : 0) const amount = offrampType == OfframpType.CASHOUT ? parseFloat(usdValue ?? '0') : tokenPrice && claimLinkData ? tokenPrice * parseFloat(claimLinkData.tokenAmount) : 0 return { totalFees, amount, hasError: amount <= totalFees, errorMessage: amount <= totalFees ? 'Transaction fees exceed the withdrawal amount. Please try a larger amount.' : undefined } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/Cashout/Cashout.consts.ts(2 hunks)src/components/Cashout/Cashout.tsx(3 hunks)src/components/Cashout/Components/Initial.view.tsx(4 hunks)src/components/Global/FeeDescription/index.tsx(1 hunks)src/components/Offramp/Confirm.view.tsx(6 hunks)src/components/Offramp/Offramp.consts.ts(1 hunks)src/components/Offramp/PromoCodeChecker.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Offramp/PromoCodeChecker.tsx
🔇 Additional comments (8)
src/components/Cashout/Cashout.consts.ts (1)
71-72: LGTM! Interface extension is well-structured.The new optional properties for gas cost estimation follow React patterns and use appropriate types.
src/components/Offramp/Offramp.consts.ts (1)
66-66: LGTM! Consistent interface extension.The estimatedGasCost property maintains type consistency with the Cashout interface.
src/components/Cashout/Cashout.tsx (1)
61-61: LGTM! State management is properly implemented.The estimatedGasCost state is initialized appropriately and correctly passed to child components.
Also applies to: 183-184
src/components/Cashout/Components/Initial.view.tsx (2)
3-21: LGTM! Imports are well-organized.The new imports are properly organized and necessary for the added functionality.
35-35: LGTM! Clean prop integration.The setEstimatedGasCost prop is well-integrated with the component's existing patterns.
src/components/Global/FeeDescription/index.tsx (1)
10-25: LGTM! Well-structured type definitions.The type definitions are clear, properly typed, and follow TypeScript best practices. The optional props are correctly marked with
?.src/components/Offramp/Confirm.view.tsx (2)
772-830: LGTM! Clean implementation of fee display.The implementation of InfoRow and FeeDescription components provides a clear and structured way to display fee information to users.
852-852: LGTM! Proper error handling in button state.The button's disabled state correctly considers both loading and error states.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (2)
106-155: Extract fee calculation logic into a separate utility file.The fee calculation logic is complex enough to warrant its own utility file. This would improve maintainability and make the logic reusable across different components.
Consider creating a new file
src/utils/fee.utils.ts:// Percentage of maximum network fee to use as expected fee (70%) export const EXPECTED_NETWORK_FEE_MULTIPLIER = 0.7 // Percentage of maximum slippage to use as expected slippage (10%) export const EXPECTED_SLIPPAGE_MULTIPLIER = 0.1 export interface FeeCalculationResult { networkFee: { expected: string; max: string } slippage?: { expected: string; max: string } estimatedFee: string totalMax: string } export const calculateFees = (params: { isXChain: boolean txFee: string estimatedGasCost: string calculatedSlippage: string | null tokenPriceData: any requestedAmount: string }): FeeCalculationResult => { // Move the calculation logic here }
440-468: Consider using React Context for fee-related data.The components receive multiple fee-related props, which could lead to prop drilling. Consider creating a FeeContext to manage fee-related state and calculations.
Example implementation:
// src/context/feeContext.tsx export const FeeContext = React.createContext<{ loading: boolean estimatedFee: string networkFee: string slippageRange?: { min: string; max: string } }>({}) export const FeeProvider: React.FC = ({ children }) => { // Move fee calculation logic here return <FeeContext.Provider value={...}>{children}</FeeContext.Provider> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Offramp/Confirm.view.tsx(7 hunks)src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Create/Link/Confirm.view.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/Offramp/Confirm.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🪛 Biome (1.9.4)
src/components/Offramp/Confirm.view.tsx
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (1)
156-163: 🛠️ Refactor suggestionEnhance error handling for fee calculation failures.
The button disabled state should also consider fee calculation errors. Currently, it only checks if the estimatedFee exists.
const isButtonDisabled = useMemo(() => { return ( viewState === ViewState.LOADING || viewState === ViewState.ERROR || - (viewState === ViewState.READY_TO_PAY && !feeCalculations.estimatedFee) + (viewState === ViewState.READY_TO_PAY && (!feeCalculations.estimatedFee || feeCalculations.error)) ) -}, [viewState, isLoading, feeCalculations.estimatedFee]) +}, [viewState, isLoading, feeCalculations.estimatedFee, feeCalculations.error])Likely invalid or redundant comment.
src/components/Offramp/Confirm.view.tsx (1)
671-702:⚠️ Potential issueReplace unsafe isNaN with Number.isNaN.
The global
isNaNfunction performs type coercion and is considered unsafe.- if (!isNaN(amount) && !isNaN(totalFees) && amount <= totalFees) { + if (!Number.isNaN(amount) && !Number.isNaN(totalFees) && amount <= totalFees) {Also, consider adding validation for negative values:
+ if (amount < 0 || totalFees < 0) { + setErrorState({ + showError: true, + errorMessage: 'Invalid amount or fees detected.', + }) + return + }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
- update icons - implement visual difference for dropdown items - implement cashout feedback
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/components/Offramp/Confirm.view.tsx (3)
657-669: 🛠️ Refactor suggestionExtract fee constants and add TypeScript types.
The function uses hardcoded fee values and lacks type safety.
Apply the suggested changes from the past review to improve maintainability and type safety.
671-702:⚠️ Potential issueReplace unsafe
isNaNwithNumber.isNaN.The current implementation uses the global
isNaNfunction which performs type coercion.Apply the suggested fix from the static analysis hint to use
Number.isNaNinstead.🧰 Tools
🪛 Biome (1.9.4)
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
796-851: 🛠️ Refactor suggestionReuse fee calculation logic in UI components.
The fee calculations are duplicated in the InfoRow and FeeDescription components.
Apply the suggested changes from the past review to maintain consistency and reduce code duplication.
🧹 Nitpick comments (3)
src/components/Global/InfoRow/index.tsx (1)
14-31: Consider adding aria-label for better accessibility.The component's UI elements could benefit from improved accessibility.
Apply this diff to enhance accessibility:
- <div className={'flex w-full items-center justify-between gap-1 px-2 text-h8 text-gray-1'}> + <div + className={'flex w-full items-center justify-between gap-1 px-2 text-h8 text-gray-1'} + role="group" + aria-label={`${label} information row`} + > <div className="flex items-center gap-1"> <Icon name={iconName} className="h-4 fill-gray-1" /> - <label className={twMerge('text-h8 font-bold', smallFont ? 'text-h9' : 'text-sm')}>{label}</label> + <label + className={twMerge('text-h8 font-bold', smallFont ? 'text-h9' : 'text-sm')} + aria-label={label} + > + {label} + </label> </div>src/components/Global/FeeDescription/index.tsx (2)
74-77: Improve currency value formatting consistency.The currency formatting logic could be more consistent and reusable.
Extract the currency formatting logic into a utility function:
+const formatCurrencyValue = (value: number | string): string => { + const numValue = parseFloat(value.toString()) + return numValue < 0.01 ? '< $ 0.01' : `~ $ ${formatAmount(numValue)}` +} + <span className="text-sm font-normal"> - {parseFloat(estimatedFee.toString()) < 0.01 - ? '< $ 0.01' - : `~ $ ${formatAmount(estimatedFee)}`} + {formatCurrencyValue(estimatedFee)} </span>
88-91: Consider using CSS transition classes for better performance.The current transition implementation might cause layout thrashing.
Apply this diff to improve transition performance:
- className={`overflow-hidden transition-all duration-300 ease-in-out ${ - toggleDetailedView.isExpanded ? 'max-h-60 opacity-100' : 'max-h-0 opacity-0' - }`} + className={`overflow-hidden transition-[max-height,opacity] duration-300 ease-in-out ${ + toggleDetailedView.isExpanded ? 'max-h-60 opacity-100' : 'max-h-0 opacity-0' + }`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Global/FeeDescription/index.tsx(1 hunks)src/components/Global/InfoRow/index.tsx(1 hunks)src/components/Offramp/Confirm.view.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Offramp/Confirm.view.tsx
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 682-682: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
src/components/Global/InfoRow/index.tsx (1)
5-12: LGTM! Well-structured props interface.The
InfoRowPropsinterface is well-defined with appropriate types and clear naming conventions.src/components/Global/FeeDescription/index.tsx (1)
10-25: LGTM! Well-structured type definitions.The type definitions are clear and follow TypeScript best practices.
src/components/Offramp/Confirm.view.tsx (1)
873-873: LGTM! Proper error handling in button state.The button is correctly disabled when loading or when there's an error state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Offramp/Confirm.view.tsx (1)
70-74: Consider extracting account matching logic into a helper function.The account matching logic uses multiple string operations that could be simplified and made more maintainable.
+const normalizeAccountIdentifier = (identifier: string): string => + identifier?.toLowerCase().replace(/\s+/g, ''); -const accountType = user?.accounts?.find( - (account) => - account?.account_identifier?.replaceAll(/\s/g, '').toLowerCase() === - offrampForm.recipient?.replaceAll(/\s/g, '').toLowerCase() -)?.account_type +const accountType = user?.accounts?.find( + (account) => + normalizeAccountIdentifier(account?.account_identifier) === + normalizeAccountIdentifier(offrampForm.recipient) +)?.account_type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Cashout/Cashout.consts.ts(3 hunks)src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Offramp/Confirm.view.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Create/Link/Confirm.view.tsx
- src/components/Cashout/Cashout.consts.ts
🧰 Additional context used
📓 Learnings (1)
src/components/Offramp/Confirm.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
🪛 Biome (1.9.4)
src/components/Offramp/Confirm.view.tsx
[error] 695-695: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 695-695: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (4)
src/components/Offramp/Confirm.view.tsx (4)
6-8: LGTM! New components imported for fee display.The addition of
FeeDescriptionandInfoRowcomponents aligns with the PR's objective of improving fee calculations and display.
891-891: LGTM! Button disabled state prevents invalid transactions.The button's disabled state correctly prevents transactions when there are errors or loading is in progress.
682-715:⚠️ Potential issueReplace unsafe isNaN with Number.isNaN.
The current implementation uses the global
isNaNfunction which performs type coercion.- if (!isNaN(amount) && !isNaN(totalFees) && amount <= totalFees) { + if (!Number.isNaN(amount) && !Number.isNaN(totalFees) && amount <= totalFees) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 695-695: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 695-695: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
663-679: 🛠️ Refactor suggestionDocument fee calculation constants and improve type safety.
The fee calculation logic uses magic numbers and lacks proper TypeScript types.
+// Constants for fee calculations +const FEE_CONSTANTS = { + IBAN_ACCOUNT: 1, + DEFAULT_ACCOUNT: 0.5, + CROSS_CHAIN_MULTIPLIER: 0.1, // 10% of gas cost for cross-chain transactions +} as const; +type AccountType = 'iban' | string | undefined; const calculateEstimatedFee = ( estimatedGasCost: string | undefined, hasPromoCode: boolean, - accountType: string | undefined, + accountType: AccountType, hasCrossChain: boolean ): number => { const baseGasCost = parseFloat(estimatedGasCost ?? '0') if (hasPromoCode) return baseGasCost - const accountFee = accountType === 'iban' ? 1 : 0.5 - const crossChainFee = hasCrossChain ? baseGasCost * 0.1 : 0 + const accountFee = accountType === 'iban' ? FEE_CONSTANTS.IBAN_ACCOUNT : FEE_CONSTANTS.DEFAULT_ACCOUNT + const crossChainFee = hasCrossChain ? baseGasCost * FEE_CONSTANTS.CROSS_CHAIN_MULTIPLIER : 0 return baseGasCost + accountFee + crossChainFee }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
Line range hint
856-914: Reuse existing fee calculations to avoid duplication.The fee calculation logic is duplicated in the UI components. Consider reusing the
feeCalculationsobject to maintain consistency and reduce code duplication.-const bankingFee = appliedPromoCode ? 0 : accountType === 'iban' ? 1 : 0.5 -const totalFees = bankingFee + (calculatedSlippage?.expected ?? 0) +const totalFees = feeCalculations.networkFee.expected + + (feeCalculations.slippage?.expected ?? 0)
🧹 Nitpick comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
103-104: Add input validation and documentation for slippage calculation.The slippage calculation should validate inputs to prevent negative values and include a comment explaining the formula.
+// Calculate slippage in USD based on token price and percentage +// Formula: (slippage_percentage/100) * token_price * token_amount +const slippageAmount = Math.max( + 0, + ((slippagePercentage / 100) * selectedTokenData.price * Number(estimatedFromValue)).toFixed(2) +) -return ((slippagePercentage / 100) * selectedTokenData.price * Number(estimatedFromValue)).toFixed(2) +return slippageAmount
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Offramp/Confirm.view.tsx(13 hunks)src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (4)
src/components/Request/Pay/Views/Initial.view.tsx (2)
442-470: LGTM! Well-structured fee display components.The implementation provides clear fee information with proper loading states and helpful tooltips.
108-157: 🛠️ Refactor suggestionExtract fee calculation constants and improve modularity.
The fee calculation logic uses magic numbers and could be more modular. Consider extracting these into named constants and breaking down the calculations into smaller functions.
+// Fee calculation constants +const FEE_MULTIPLIERS = { + // Percentage of maximum network fee to use as expected fee + EXPECTED_NETWORK_FEE: 0.7, + // Percentage of maximum slippage to use as expected slippage + EXPECTED_SLIPPAGE: 0.1, +} as const; +// Helper functions for fee calculations +const calculateNetworkFee = (isXChain: boolean, txFee: string, estimatedGasCost: string) => ({ + expected: isXChain + ? Number(txFee) * FEE_MULTIPLIERS.EXPECTED_NETWORK_FEE + : Number(estimatedGasCost) * FEE_MULTIPLIERS.EXPECTED_NETWORK_FEE, + max: isXChain ? Number(txFee) : Number(estimatedGasCost), +}); +const calculateSlippage = (calculatedSlippage: string | null) => + calculatedSlippage ? { + expected: Number(calculatedSlippage) * FEE_MULTIPLIERS.EXPECTED_SLIPPAGE, + max: Number(calculatedSlippage), + } : undefined; const feeCalculations = useMemo(() => { - const EXPECTED_NETWORK_FEE_MULTIPLIER = 0.7 - const EXPECTED_SLIPPAGE_MULTIPLIER = 0.1 - - const networkFee = { - expected: isXChain - ? Number(txFee) * EXPECTED_NETWORK_FEE_MULTIPLIER - : Number(estimatedGasCost) * EXPECTED_NETWORK_FEE_MULTIPLIER, - max: isXChain ? Number(txFee) : Number(estimatedGasCost), - } + const networkFee = calculateNetworkFee(isXChain, txFee, estimatedGasCost); - const slippage = calculatedSlippage - ? { - expected: Number(calculatedSlippage) * EXPECTED_SLIPPAGE_MULTIPLIER, - max: Number(calculatedSlippage), - } - : undefined + const slippage = calculateSlippage(calculatedSlippage);Likely invalid or redundant comment.
src/components/Offramp/Confirm.view.tsx (2)
728-762:⚠️ Potential issueReplace unsafe isNaN with Number.isNaN.
The current implementation uses the global
isNaNfunction which performs type coercion. UseNumber.isNaNinstead for safer number validation.-if (!isNaN(amount) && !isNaN(totalFees) && amount <= totalFees) { +if (!Number.isNaN(amount) && !Number.isNaN(totalFees) && amount <= totalFees) {Likely invalid or redundant comment.
710-726: 🛠️ Refactor suggestionExtract fee constants and improve type safety.
The fee calculation uses magic numbers for account fees. Consider extracting these into named constants and adding proper TypeScript types.
+const ACCOUNT_FEES = { + IBAN: 1, + DEFAULT: 0.5, +} as const; +type AccountType = 'iban' | string | undefined; const calculateEstimatedFee = ( estimatedGasCost: string | undefined, hasPromoCode: boolean, - accountType: string | undefined, + accountType: AccountType, isCrossChainTx: boolean ): number => { const estimatedNetworkFee = parseFloat(estimatedGasCost ?? '0') if (hasPromoCode) return estimatedNetworkFee - const accountFee = accountType === 'iban' ? 1 : 0.5 + const accountFee = accountType === 'iban' ? ACCOUNT_FEES.IBAN : ACCOUNT_FEES.DEFAULT const crossChainFee = isCrossChainTx ? (calculatedSlippage?.expected ?? 0) : 0 return estimatedNetworkFee + accountFee + crossChainFee }Likely invalid or redundant comment.
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 (1)
src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
442-470: LGTM! Clean UI implementation for fee display.The implementation of
FeeDescriptionandInfoRowcomponents provides a clear and consistent way to display fee information. The loading states and conditional rendering are handled appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
109-158: 🛠️ Refactor suggestionCentralize fee calculation logic into a shared utility.
The fee calculation logic is well-structured but should be moved to a shared location to prevent inconsistencies across different flows (cashout, send, etc.).
Create a new utility file (e.g.,
src/utils/fee.utils.ts) and move the fee calculation logic there:// src/utils/fee.utils.ts export const FEE_CONSTANTS = { EXPECTED_NETWORK_FEE_MULTIPLIER: 0.7, // 70% of max for expected EXPECTED_SLIPPAGE_MULTIPLIER: 0.1, // 10% of max for expected } as const; interface FeeCalculationParams { isXChain: boolean; txFee: string; estimatedGasCost: string; calculatedSlippage: string | null; tokenAmount: string; tokenPrice?: number; } export function calculateFees({ isXChain, txFee, estimatedGasCost, calculatedSlippage, tokenAmount, tokenPrice, }: FeeCalculationParams) { const networkFee = { expected: isXChain ? Number(txFee) * FEE_CONSTANTS.EXPECTED_NETWORK_FEE_MULTIPLIER : Number(estimatedGasCost) * FEE_CONSTANTS.EXPECTED_NETWORK_FEE_MULTIPLIER, max: isXChain ? Number(txFee) : Number(estimatedGasCost), }; const slippage = calculatedSlippage ? { expected: Number(calculatedSlippage) * FEE_CONSTANTS.EXPECTED_SLIPPAGE_MULTIPLIER, max: Number(calculatedSlippage), } : undefined; const requestedAmountUSD = tokenPrice ? Number(tokenAmount) * tokenPrice : Number(tokenAmount); const totalMax = requestedAmountUSD + networkFee.max + (slippage?.max || 0); return { networkFee: { expected: formatNumberSafely(networkFee.expected), max: formatNumberSafely(networkFee.max), }, slippage: slippage ? { expected: slippage.expected, max: slippage.max, } : undefined, estimatedFee: formatNumberSafely(networkFee.expected + (slippage?.max || 0)), totalMax: formatNumberSafely(totalMax), }; } function formatNumberSafely(num: number) { return formatAmount(num < 0.01 && num > 0 ? 0.01 : num); }
🧹 Nitpick comments (1)
src/components/Request/Pay/Views/Initial.view.tsx (1)
Line range hint
328-391: Add error recovery mechanism for payment failures.The payment handling has been improved with the isPayInitiatedByUser flag, but there's no mechanism to retry failed payments or recover from errors.
Consider adding a retry mechanism:
} catch (error) { const errorString = ErrorHandler(error) setErrorState({ showError: true, errorMessage: errorString, }) console.error('Error while submitting request link fulfillment:', error) + // Add retry button to UI when error occurs + setViewState(ViewState.ERROR_RETRYABLE) } finally { setIsPayInitiatedByUser(false) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Request/Pay/Views/Initial.view.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
🔇 Additional comments (3)
src/components/Request/Pay/Views/Initial.view.tsx (3)
3-4: LGTM! New imports and state management look good.The addition of FeeDescription and InfoRow components, along with the isPayInitiatedByUser state, improves the organization of fee-related information and loading states.
Also applies to: 100-100
159-166: LGTM! Button state management is improved.The button disabled state now correctly depends on the fee calculations, ensuring better UX by preventing actions when fees aren't properly calculated.
446-467: LGTM! Fee display components are well integrated.The FeeDescription and InfoRow components provide clear and consistent fee information to users. The loading states are properly handled using the isPayInitiatedByUser flag.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (1)
102-107: Move slippage calculation to the centralized fee utility.For consistency, the slippage calculation should be part of the centralized fee utility.
Move this logic to the
fee.utils.tsfile:export function calculateSlippage( tokenPrice: number | undefined, slippagePercentage: number | undefined, estimatedFromValue: string ): string | null { if (!tokenPrice || !slippagePercentage) return null; return ((slippagePercentage / 100) * tokenPrice * Number(estimatedFromValue)).toFixed(2); }src/components/Claim/Link/Initial.view.tsx (1)
512-518: Consider making the sponsored transaction logic more flexible.The hardcoded
$0and sponsored message could be made more dynamic to handle future changes in fee sponsorship.Consider using a configuration:
interface FeeConfig { isSponsored: boolean; sponsorMessage?: string; } const SPONSORED_FEE_CONFIG: FeeConfig = { isSponsored: true, sponsorMessage: 'This transaction is sponsored by peanut! Enjoy!', }; // Then in the component: <InfoRow iconName="gas" label="Fees" value={feeConfig.isSponsored ? '$0' : `$${calculatedFee}`} moreInfoText={feeConfig.isSponsored ? feeConfig.sponsorMessage : 'Network fees for processing the transaction.'} loading={isXchainLoading} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/Claim/Link/Initial.view.tsx(3 hunks)src/components/Claim/Link/Onchain/Confirm.view.tsx(2 hunks)src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Global/ConfirmDetails/Index.tsx(1 hunks)src/components/Global/FeeDescription/index.tsx(1 hunks)src/components/Offramp/Confirm.view.tsx(13 hunks)src/components/Request/Pay/Views/Initial.view.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Global/ConfirmDetails/Index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Claim/Link/Onchain/Confirm.view.tsx
- src/components/Create/Link/Confirm.view.tsx
- src/components/Global/FeeDescription/index.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/Request/Pay/Views/Initial.view.tsx (4)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(...)` return strings, ensuring that `calculatedFee` consistently returns a string without the need for additional type conversion.
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#594
File: src/components/Request/Pay/Views/Initial.view.tsx:188-213
Timestamp: 2024-12-31T15:08:15.640Z
Learning: Always resolve coderabbit comments when reviewing code, since they can contain useful suggestions.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
src/components/Offramp/Confirm.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
🪛 Biome (1.9.4)
src/components/Offramp/Confirm.view.tsx
[error] 316-316: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 742-742: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 742-742: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (5)
src/components/Request/Pay/Views/Initial.view.tsx (2)
100-100: LGTM! Good UX improvement.Adding
isPayInitiatedByUserstate helps prevent unnecessary loading states when the component is initially rendered.
446-467: LGTM! Great improvement to fee presentation.The new
FeeDescriptionandInfoRowcomponents provide clear and structured information about fees, with proper loading state handling.src/components/Claim/Link/Initial.view.tsx (1)
427-427: LGTM! Consistent formatting.The token amount formatting is now more consistent with other parts of the codebase.
src/components/Offramp/Confirm.view.tsx (2)
858-881: 🛠️ Refactor suggestionReuse fee calculation logic in InfoRow component.
The fee calculations are duplicated in the InfoRow component.
Use the
calculateEstimatedFeefunction:<InfoRow iconName="money-in" label="Expected receive" value={(() => { - const bankingFee = appliedPromoCode ? 0 : accountType === 'iban' ? 1 : 0.5; - const totalFees = bankingFee + (calculatedSlippage?.expected ?? 0); + const totalFees = calculateEstimatedFee({ + estimatedGasCost, + hasPromoCode: !!appliedPromoCode, + accountType, + isCrossChainTx: !!crossChainDetails, + calculatedSlippage, + }); const amount = offrampType == OfframpType.CASHOUT ? parseFloat(usdValue ?? '0') : tokenPrice && claimLinkData ? tokenPrice * parseFloat(claimLinkData.tokenAmount) : 0; return amount <= totalFees ? '$0' : `$${utils.formatTokenAmount(amount - totalFees)}` || '$0'; })()} moreInfoText="Expected amount you will receive in your bank account. You'll receive funds in your local currency." />Likely invalid or redundant comment.
729-762:⚠️ Potential issueReplace unsafe
isNaNwithNumber.isNaN.The current implementation uses the global
isNaNfunction which performs type coercion.Apply this diff to fix the issue:
- if (!isNaN(amount) && !isNaN(totalFees) && amount <= totalFees) { + if (!Number.isNaN(amount) && !Number.isNaN(totalFees) && amount <= totalFees) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 742-742: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 742-742: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
fixes TASK-8331 TASK-6946
Summary by CodeRabbit
Summary by CodeRabbit
New Features
FeeDescriptionandInfoRow) for displaying detailed transaction fee information.InfoRowcomponent.estimatedGasCostin various components for better financial detail management.Bug Fixes