Refactor/ternary confirm view and abstract fee explainers #426
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Offramp/Confirm.view.tsx (2)
71-108: Good addition of utility variables for improved readability.The introduction of utility variables for fee calculations and amount computations enhances code readability and maintainability. This centralization of logic makes it easier to understand and modify in the future.
A few suggestions for further improvement:
- Consider moving the hardcoded fee values ($1 and $0.50) to constants or configuration files for easier maintenance.
- The TODO comment about standardizing the account type is a good practice. Make sure to address this in future iterations.
To improve maintainability, consider extracting the fee values to constants:
const SEPA_FEE = 1; const ACH_FEE = 0.5; // Then use these constants in your calculations fee = accountType === 'iban' ? SEPA_FEE : ACH_FEE;
678-680: Improved render logic with dynamic fee and amount calculations.The changes to use dynamically calculated
feeandamountReceivedvalues in the render logic are good improvements. They enhance the accuracy and flexibility of the component by reflecting the actual calculated values.For consistency in formatting, consider using template literals for the fee display:
${`$${fee.toFixed(2)}`}This ensures that the fee is always displayed with two decimal places, matching the format of the
amountReceived.Also applies to: 694-696
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Offramp/Confirm.view.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/components/Offramp/Confirm.view.tsx (1)
Line range hint
1-724: Overall improvements in fee handling and amount calculations.The changes in this file focus on improving the fee handling and amount calculations for the offramp confirmation view. The introduction of utility variables and the use of dynamic calculations in the render logic enhance the component's flexibility and accuracy.
Some minor suggestions have been made for further improvements, but overall, these changes are a positive step towards better code organization and maintainability.
There was a problem hiding this comment.
I think it makes little sense to refactor this in confirm view but not in success view (where we also have these fees)
Imo: extend this PR to also cover confirm success view (and ideally move values to consts file)
wdyt @panosfilianos ?
| // TODO: standardize this type | ||
| let accountType = user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)?.account_type | ||
|
|
||
| // TODO: remove hardcoding fee amounts from here and refactor to consts file |
There was a problem hiding this comment.
praise: good todo comment.
These should prob be pulled from Cashout.consts.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/components/Offramp/Offramp.consts.ts (1)
30-32: LGTM! Consider reviewing claim link explainer wording.The addition of fee explainer constants is beneficial for maintaining consistent messaging across the application. The use of template literals for SEPA and ACH explainers ensures that the displayed fee amounts always match the defined constants.
Consider reviewing the wording of the
claimLinkFeeExplainer. The current message "Woop Woop free offramp!" is quite informal and might not be suitable for all contexts or users. You might want to consider a more professional tone, such as "No fee charged for claim link transactions."src/components/Offramp/Success.view.tsx (1)
32-43: LGTM: Improved code modularity and readability.The use of utility functions
returnOfframpFeeandreturnOfframpFeeExplainerimproves code modularity and reusability. The simplified logic for determiningaccountTypeis more readable than the previous nested optional chaining approach.Consider using optional chaining for the
accountTypedetermination to handle potential undefined values more gracefully:let accountType = user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)?.account_typeThis change would make the code more robust against potential runtime errors if
useroruser.accountsis undefined.src/utils/offramp.utils.ts (1)
532-539: LGTM: Fee calculation logic is sound, but consider explicit handling of all offramp types.The
returnOfframpFeefunction correctly calculates fees for the CASHOUT offramp type based on the account type. Using constants for fee values is a good practice for maintainability.Consider explicitly handling all offramp types for better clarity and future-proofing. For example:
export function returnOfframpFee(offrampType: OfframpType, accountType: any): number { let fee = 0; if (offrampType == OfframpType.CASHOUT) { fee = accountType === 'iban' ? OFFRAMP_IBAN_FEE_USD : OFFRAMP_NON_IBAN_FEE_USD - } + } else if (offrampType == OfframpType.CLAIM) { + fee = 0; // Explicitly set fee to 0 for CLAIM + } else { + console.warn(`Unhandled offramp type: ${offrampType}`); + } // other types of offramp (eg. CLAIM link) do not have a fee return fee; }This change makes the function's behavior more explicit for all offramp types and adds a warning for any unhandled types.
src/components/Offramp/Confirm.view.tsx (2)
93-104: Ensureamountis not negative after fee deductionSubtracting
feefrom the parsed values might result in a negativeamount, which could be unintended and misleading to users. Consider adding a check to preventamountfrom being negative.For example:
// After calculating amount amount = Math.max(0, amount);
689-691: Check for coherent message construction and potential grammar issuesIn the concatenation
feeExplainer + ' This will be deducted of the amount you will receive.', ensure that the resulting message is grammatically correct and clear to the user. It might be better to revise the wording for clarity.Consider rephrasing:
- text={feeExplainer + ' This will be deducted of the amount you will receive.'} + text={feeExplainer + ' This amount will be deducted from the amount you will receive.'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/components/Offramp/Confirm.view.tsx (4 hunks)
- src/components/Offramp/Offramp.consts.ts (1 hunks)
- src/components/Offramp/Success.view.tsx (2 hunks)
- src/utils/index.ts (1 hunks)
- src/utils/offramp.utils.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
src/utils/index.ts (1)
4-4: LGTM! Verify impact and update documentation.The change from 'cashout.utils' to 'offramp.utils' aligns with the PR objectives. This generalization likely improves code reusability for different offramp scenarios.
Please ensure:
- All imports of 'cashout.utils' throughout the codebase have been updated to use 'offramp.utils'.
- Any documentation referencing 'cashout.utils' is updated to reflect this change.
Run the following script to verify the changes:
This will help identify any places that might need updating due to this change.
✅ Verification successful
Verified: All 'cashout.utils' references have been successfully removed and replaced with 'offramp.utils'.
The shell scripts confirmed that there are no remaining references to
cashout.utilsin both TypeScript and Markdown files. The newofframp.utilsexport is present as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to 'cashout.utils' and new usage of 'offramp.utils' echo "Checking for remaining 'cashout.utils' references:" rg --type typescript "cashout\.utils" echo "Checking new 'offramp.utils' usage:" rg --type typescript "offramp\.utils"Length of output: 353
Script:
#!/bin/bash # Description: Check for any remaining references to 'cashout.utils' and new usage of 'offramp.utils' in TypeScript files echo "Checking for remaining 'cashout.utils' references:" rg "cashout\.utils" --glob "*.ts" --glob "*.tsx" echo "Checking new 'offramp.utils' usage:" rg "offramp\.utils" --glob "*.ts" --glob "*.tsx"Length of output: 356
Script:
#!/bin/bash # Description: Check for any remaining references to 'cashout.utils' in Markdown documentation files echo "Checking for remaining 'cashout.utils' references in Markdown files:" rg "cashout\.utils" --glob "*.md"Length of output: 188
src/components/Offramp/Offramp.consts.ts (1)
27-28: LGTM! Verify fee amounts.The addition of constants for IBAN and non-IBAN transaction fees is a good practice. It centralizes the fee information and makes it easier to update if needed.
Please confirm that the fee amounts ($1 for IBAN and $0.5 for non-IBAN) are correct and up-to-date with your current pricing model.
src/components/Offramp/Success.view.tsx (3)
20-24: LGTM: Improved code structure and readability.The addition of comments and blank lines enhances the code's readability by clearly separating different sections. The use of destructuring for the
useAuthhook is a good React practice.
25-31: LGTM: Well-structured conditional logic for CLAIM offramp.The conditional logic for setting
blockExplorerUrlis well-implemented. The use of optional chaining in the condition is a good practice for handling potential undefined values. The comments clearly separate the logic for different offramp types, improving code readability.
99-102: LGTM: Simplified fee display logic.The rendering logic for the fee and its explanation has been effectively simplified. The use of pre-calculated
feeandfeeExplainervariables improves code readability and maintainability. The consistent use of the MoreInfo component for additional information is a good practice.src/utils/offramp.utils.ts (2)
7-7: LGTM: New imports are appropriate for the added functionality.The new imports are relevant to the offramp fee calculation and explanation functions being added. They include necessary constants, types, and explanatory text.
531-551: Overall, good addition of offramp fee utilities with minor improvement suggestions.The new functions
returnOfframpFeeandreturnOfframpFeeExplainerare valuable additions to the offramp utilities. They correctly handle fee calculation and explanation for different offramp types and account types. The use of constants and imported explanatory text promotes maintainability.Consider implementing the suggested improvements to enhance readability, maintainability, and robustness:
- Explicit handling of all offramp types in
returnOfframpFee.- Refactoring
returnOfframpFeeExplainerto use a switch statement for clearer type handling.These changes will make the code more robust and easier to extend in the future.
src/components/Offramp/Confirm.view.tsx (1)
673-675: Verify thatfeeandfeeExplainerare defined before renderingEnsure that
feeandfeeExplainerhave valid values to prevent rendering issues or displayingundefinedin the UI. If there's a possibility they areundefined, provide default values or handle these cases appropriately.
| export function returnOfframpFeeExplainer(offrampType: OfframpType, accountType: any): string { | ||
| let feeExplainer = '' | ||
| if (offrampType == OfframpType.CASHOUT) { | ||
| feeExplainer = accountType === 'iban' ? sepaFeeExplainer + ' ' + achFeeExplainer: achFeeExplainer + ' ' + sepaFeeExplainer | ||
| } else { | ||
| if (offrampType == OfframpType.CLAIM) { | ||
| feeExplainer = claimLinkFeeExplainer | ||
| } | ||
| } | ||
| return feeExplainer | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function structure for better readability and maintainability.
The returnOfframpFeeExplainer function correctly provides fee explanations for different offramp types. However, the structure can be improved for better readability and maintainability.
Consider refactoring the function as follows:
export function returnOfframpFeeExplainer(offrampType: OfframpType, accountType: any): string {
switch (offrampType) {
case OfframpType.CASHOUT:
return accountType === 'iban'
? `${sepaFeeExplainer} ${achFeeExplainer}`
: `${achFeeExplainer} ${sepaFeeExplainer}`;
case OfframpType.CLAIM:
return claimLinkFeeExplainer;
default:
console.warn(`Unhandled offramp type: ${offrampType}`);
return '';
}
}This refactored version:
- Uses a switch statement for clearer handling of different offramp types.
- Removes unnecessary variable assignment.
- Handles unrecognized offramp types with a warning.
- Is more concise and easier to extend in the future.
| let accountType = user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)?.account_type | ||
| const fee = utils.returnOfframpFee( | ||
| offrampType, | ||
| accountType | ||
| ); |
There was a problem hiding this comment.
Handle undefined accountType to prevent potential errors
If accountType is undefined (e.g., if no matching account is found), passing it to utils.returnOfframpFee and utils.returnOfframpFeeExplainer might lead to unexpected results. Consider providing a default value or adding a check to ensure accountType is defined.
Here's a possible fix:
let accountType = user?.accounts.find((account) =>
account.account_identifier === offrampForm.recipient
)?.account_type;
+if (!accountType) {
+ // Handle undefined accountType, e.g., set a default or show an error message
+ accountType = 'defaultAccountType'; // Replace with an appropriate default
+}
const fee = utils.returnOfframpFee(
offrampType,
accountType
);Committable suggestion was skipped due to low confidence.
| amount = parseFloat(usdValue ?? tokenValue ?? '') - fee | ||
| } else { | ||
| amount = parseFloat(usdValue ?? '') - fee | ||
| } |
There was a problem hiding this comment.
Add validation to prevent NaN in amount calculation
When both usdValue and tokenValue are undefined or empty strings, parseFloat('') returns NaN, which may cause amount to be NaN. This could lead to incorrect calculations or displays. Ensure that valid numeric values are being parsed.
You can modify the code as follows:
if (accountType == 'iban') {
- amount = parseFloat(usdValue ?? tokenValue ?? '') - fee
+ const value = usdValue ?? tokenValue;
+ if (value) {
+ amount = parseFloat(value) - fee;
+ } else {
+ amount = 0;
+ }
} else {
- amount = parseFloat(usdValue ?? '') - fee
+ if (usdValue) {
+ amount = parseFloat(usdValue) - fee;
+ } else {
+ amount = 0;
+ }
}📝 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.
| amount = parseFloat(usdValue ?? tokenValue ?? '') - fee | |
| } else { | |
| amount = parseFloat(usdValue ?? '') - fee | |
| } | |
| if (accountType == 'iban') { | |
| const value = usdValue ?? tokenValue; | |
| if (value) { | |
| amount = parseFloat(value) - fee; | |
| } else { | |
| amount = 0; | |
| } | |
| } else { | |
| if (usdValue) { | |
| amount = parseFloat(usdValue) - fee; | |
| } else { | |
| amount = 0; | |
| } | |
| } |
|
This branch has a file conflict w/ |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/Offramp/Confirm.view.tsx (4 hunks)
- src/components/Offramp/Success.view.tsx (3 hunks)
- src/utils/offramp.utils.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Offramp/Success.view.tsx
🧰 Additional context used
🔇 Additional comments (4)
src/components/Offramp/Confirm.view.tsx (4)
20-21: LGTM: New imports for fee-related constantsThe addition of new constants for fee explanations and offramp fees is a good practice. It centralizes the configuration and makes it easier to maintain and update fee-related information.
Also applies to: 25-26, 30-31
79-85: LGTM: Introduction of utility variables for fee and account typeThe addition of utility variables for account type, fee, and fee explainer improves code readability and maintainability. Using utility functions from a separate file is a good practice for keeping the component code clean.
671-673: LGTM: Improved fee and total amount displayThe updates to the fee and total amount displays using the new utility variables enhance the user experience by providing more accurate and dynamic information. The addition of MoreInfo components for fee explanations is a good way to offer users additional context.
Also applies to: 687-690
81-85:⚠️ Potential issueHandle potential undefined
accountTypeThe
accountTypevariable might be undefined if no matching account is found. This could lead to unexpected behavior when passing it toutils.returnOfframpFee.Consider adding a fallback or default value for
accountType:let accountType = user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)?.account_type +accountType = accountType || 'default' // Replace 'default' with an appropriate fallback value const fee = utils.returnOfframpFee( offrampType, accountType );Likely invalid or redundant comment.
| // type: 'iban' or other | ||
| // TODO: standardize this type | ||
| let accountType = user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)?.account_type | ||
| const fee = utils.returnOfframpFee( | ||
| offrampType, | ||
| accountType | ||
| ); | ||
| const feeExplainer = utils.returnOfframpFeeExplainer( | ||
| offrampType, | ||
| accountType | ||
| ) | ||
|
|
||
| const amount = utils.returnOfframpTotalAmountAfterFees( | ||
| offrampType, | ||
| accountType, | ||
| usdValue, | ||
| tokenValue, | ||
| fee, | ||
| tokenPrice, | ||
| claimLinkData?.tokenAmount | ||
| ) | ||
|
|
||
| const amountReceived = utils.formatTokenAmount(amount) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider further optimizations and error handling
The changes improve the fee calculations and displays, enhancing the component's functionality. To further improve the code, consider the following suggestions:
- Memoize complex calculations using
useMemoto optimize performance, especially if the component re-renders frequently. - Add error handling for cases where required data (e.g.,
user,offrampForm) might be undefined. - Consider extracting the fee and amount calculation logic into a custom hook to improve reusability and testability.
Example of using useMemo for fee calculation:
const fee = useMemo(() => {
if (!accountType) return 0;
return utils.returnOfframpFee(offrampType, accountType);
}, [offrampType, accountType]);| @@ -527,3 +528,47 @@ export const fetchRouteRaw = async ( | |||
| return undefined | |||
| } | |||
| } | |||
|
|
|||
| export function returnOfframpFee(offrampType: OfframpType, accountType: any): number { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific types instead of 'any' for 'accountType' parameter
Using any for the accountType parameter can lead to type safety issues and makes the code less self-documenting. Consider defining a specific type or enum for accountType to enhance type safety and code clarity.
Proposed change:
Define a type for accountType:
type AccountType = 'iban' | 'us';Update the function signatures:
-export function returnOfframpFee(offrampType: OfframpType, accountType: any): number {
+export function returnOfframpFee(offrampType: OfframpType, accountType: AccountType): number {Apply similar changes to the other functions:
-export function returnOfframpFeeExplainer(offrampType: OfframpType, accountType: any): string {
+export function returnOfframpFeeExplainer(offrampType: OfframpType, accountType: AccountType): string {-export function returnOfframpTotalAmountAfterFees(
+export function returnOfframpTotalAmountAfterFees(
offrampType: OfframpType,
- accountType: any,
+ accountType: AccountType,
...
): number {Also applies to: 541-541, 553-553
| amount = parseFloat(usdValue ?? tokenValue ?? '') - fee | ||
| } else { | ||
| amount = parseFloat(usdValue ?? '') - fee | ||
| } | ||
| } else if (offrampType == OfframpType.CLAIM && tokenPrice && tokenAmount) { | ||
| amount = tokenPrice * parseFloat(tokenAmount) - fee |
There was a problem hiding this comment.
Handle potential NaN values when parsing amounts
When usdValue, tokenValue, or tokenAmount are undefined or empty strings, parseFloat will return NaN, which can lead to incorrect calculations and results. Ensure that these values are validated before parsing and provide default values where appropriate.
Proposed fix:
Add default values and validate parsed numbers:
const usdAmount = parseFloat(usdValue ?? tokenValue ?? '0') || 0;
const tokenAmountParsed = parseFloat(tokenAmount ?? '0') || 0;Update the calculations:
-amount = parseFloat(usdValue ?? tokenValue ?? '') - fee
+amount = usdAmount - fee-amount = tokenPrice * parseFloat(tokenAmount) - fee
+amount = tokenPrice * tokenAmountParsed - feeAdditionally, you might want to handle cases where tokenPrice is undefined:
const effectiveTokenPrice = tokenPrice || 0;
amount = effectiveTokenPrice * tokenAmountParsed - fee;|
|
||
| export function returnOfframpFee(offrampType: OfframpType, accountType: any): number { | ||
| let fee = 0; | ||
| if (offrampType == OfframpType.CASHOUT) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use strict equality checks ('===') instead of loose equality ('==')
For better type safety and to avoid unexpected type coercion, it's recommended to use strict equality checks (===) in TypeScript when comparing values.
Apply this change to all relevant comparisons:
-if (offrampType == OfframpType.CASHOUT) {
+if (offrampType === OfframpType.CASHOUT) {-if (accountType == 'iban') {
+if (accountType === 'iban') {-if (offrampType == OfframpType.CLAIM && tokenPrice && tokenAmount) {
+if (offrampType === OfframpType.CLAIM && tokenPrice && tokenAmount) {Also applies to: 543-543, 564-564, 570-570
This refers to #400 (comment)