Skip to content

Refactor/ternary confirm view and abstract fee explainers #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Refactor/ternary confirm view and abstract fee explainers #426

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b769a2b
refactor: Confirm view cashout calculations
panosfilianos Oct 8, 2024
f064a2d
refactor: Clean up unused code and refactor fee explainers
panosfilianos Oct 8, 2024
a6b8fec
refactor: Move fee amounts to consts
panosfilianos Oct 15, 2024
bb5201f
chore: Remove TODO
panosfilianos Oct 15, 2024
0b9c025
refactor: Move fee explainers to consts
panosfilianos Oct 15, 2024
9b51ea7
refactor: Change cashout.utils -> offramp.utils to keep naming consis…
panosfilianos Oct 15, 2024
f6f586a
refactor: Abstract fee and explainer logic to offramp.utils to use in…
panosfilianos Oct 15, 2024
bf8b7f1
refactor: Calculate fees in Success view through utility functions
panosfilianos Oct 15, 2024
a709c6e
refactor: Calculate amount in utils function and update feeExplainer …
panosfilianos Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 38 additions & 53 deletions src/components/Offramp/Confirm.view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,17 @@ import useClaimLink from '@/components/Claim/useClaimLink'
import Link from 'next/link'
import { CrispButton } from '@/components/CrispChat'
import {
achFeeExplainer,
claimLinkFeeExplainer,
CrossChainDetails,
IOfframpConfirmScreenProps,
LiquidationAddress,
OFFRAMP_IBAN_FEE_USD,
OFFRAMP_NON_IBAN_FEE_USD,
OfframpType,
optimismChainId,
PeanutAccount,
sepaFeeExplainer,
usdcAddressOptimism,
} from '@/components/Offramp/Offramp.consts'
import { FAQComponent } from '../Cashout/Components/Faq.comp'
Expand Down Expand Up @@ -68,6 +73,35 @@ export const OfframpConfirmView = ({
//////////////////////
// state and context vars for claim link offramp

//////////////////////
// utility JSX vars

// 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
);
Comment on lines +81 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

const feeExplainer = utils.returnOfframpFeeExplainer(
offrampType,
accountType
)

let amount: number = 0

if (offrampType == OfframpType.CASHOUT) {
if (accountType == 'iban') {
amount = parseFloat(usdValue ?? tokenValue ?? '') - fee
} else {
amount = parseFloat(usdValue ?? '') - fee
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}
}

} else if (offrampType == OfframpType.CLAIM && tokenPrice && claimLinkData) {
amount = tokenPrice * parseFloat(claimLinkData.tokenAmount) - fee
}

const amountReceived = utils.formatTokenAmount(amount)

//////////////////////
// functions w/ shared functionality across all offramp types
const { setStep: setActiveStep, activeStep } = useSteps({
Expand Down Expand Up @@ -636,41 +670,14 @@ export const OfframpConfirmView = ({
<label className="font-bold">Fee</label>
</div>
<span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4">
{user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)
?.account_type === 'iban'
? '$1'
: '$0.50'}
${fee}
<MoreInfo
text={
user?.accounts.find(
(account) => account.account_identifier === offrampForm.recipient
)?.account_type === 'iban'
? 'For SEPA transactions a fee of $1 is charged. For ACH transactions a fee of $0.50 is charged.'
: 'For ACH transactions a fee of $0.50 is charged. For SEPA transactions a fee of $1 is charged.'
}
text={feeExplainer}
/>
</span>
</div>

<div className="flex w-full flex-row items-center justify-between gap-1 px-2 text-h8 text-gray-1">
{offrampType == OfframpType.CLAIM && (
<>
<div className="flex w-max flex-row items-center justify-center gap-1">
<Icon name={'transfer'} className="h-4 fill-gray-1" />
<label className="font-bold">Total</label>
</div>
<span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4">
$
{tokenPrice &&
claimLinkData &&
utils.formatTokenAmount(
tokenPrice * parseFloat(claimLinkData.tokenAmount)
)}{' '}
<MoreInfo text={'Woop Woop free offramp!'} />
</span>
</>
)}

<div className="flex w-max flex-row items-center justify-center gap-1">
{offrampType == OfframpType.CLAIM && (
<Icon name={'transfer'} className="h-4 fill-gray-1" />
Expand All @@ -679,31 +686,9 @@ export const OfframpConfirmView = ({
</div>

<span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4">
$
{user?.accounts.find((account) => account.account_identifier === offrampForm.recipient)
?.account_type === 'iban'
? offrampType == OfframpType.CASHOUT
? utils.formatTokenAmount(parseFloat(usdValue ?? tokenValue ?? '') - 1)
: tokenPrice &&
claimLinkData &&
utils.formatTokenAmount(
tokenPrice * parseFloat(claimLinkData.tokenAmount) - 1
)
: offrampType == OfframpType.CASHOUT
? utils.formatTokenAmount(parseFloat(usdValue ?? '') - 0.5)
: tokenPrice &&
claimLinkData &&
utils.formatTokenAmount(
tokenPrice * parseFloat(claimLinkData.tokenAmount) - 0.5
)}
$ {amountReceived}
<MoreInfo
text={
user?.accounts.find(
(account) => account.account_identifier === offrampForm.recipient
)?.account_type === 'iban'
? 'For SEPA transactions a fee of $1 is charged. For ACH transactions a fee of $0.50 is charged. This will be deducted of the amount you will receive.'
: 'For ACH transactions a fee of $0.50 is charged. For SEPA transactions a fee of $1 is charged. This will be deducted of the amount you will receive.'
}
text={feeExplainer + ' This will be deducted of the amount you will receive.'}
/>
</span>
</div>
Expand Down
7 changes: 7 additions & 0 deletions src/components/Offramp/Offramp.consts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ export enum OfframpType {
CLAIM = 'CLAIM'
}

export const OFFRAMP_IBAN_FEE_USD = 1
export const OFFRAMP_NON_IBAN_FEE_USD = 0.5

export const sepaFeeExplainer = `For SEPA transactions a fee of $${OFFRAMP_IBAN_FEE_USD} is charged.`
export const achFeeExplainer = `For ACH transactions a fee of $${OFFRAMP_NON_IBAN_FEE_USD} is charged.`
export const claimLinkFeeExplainer = 'Woop Woop free offramp!'

export const MIN_CASHOUT_LIMIT = 10 // $10 minimum
export const MAX_CASHOUT_LIMIT = 101000 // $101,000 maximum

Expand Down
49 changes: 21 additions & 28 deletions src/components/Offramp/Success.view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,30 @@ export const OfframpSuccessView = ({
recipientType, // available on claims
transactionHash, // available on claims
}: _consts.IOfframpSuccessScreenProps) => {
// setup offrampType == CASHOUT props

//////////////////////
// state and context vars for cashout offramp
const { user } = useAuth()
const accountType = user?.accounts?.find(
(account) => account?.account_identifier?.toLowerCase() === offrampForm.recipient?.toLowerCase()
)?.account_type

// setup offrampType == CLAIM props
//////////////////////
// state and context vars for claim link offramp
let blockExplorerUrl: string | undefined
if (offrampType == _consts.OfframpType.CLAIM && claimLinkData) {
blockExplorerUrl = utils.getExplorerUrl(claimLinkData.chainId)
}

//////////////////////
// utility JSX vars
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
)

return (
<div className="flex w-full flex-col items-center justify-center gap-6 py-2 pb-20 text-center">
<label className="text-h2">Yay!</label>
Expand Down Expand Up @@ -84,29 +96,10 @@ export const OfframpSuccessView = ({
<label className="font-bold">Fee</label>
</div>
<span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4">
{offrampType == _consts.OfframpType.CASHOUT && (
<>
{user?.accounts?.find((account) => account.account_identifier === offrampForm.recipient)
?.account_type === 'iban'
? '$1'
: '$0.50'}
<MoreInfo
text={
user?.accounts.find(
(account) => account.account_identifier === offrampForm.recipient
)?.account_type === 'iban'
? 'For SEPA transactions a fee of $1 is charged. For ACH transactions a fee of $0.50 is charged.'
: 'For ACH transactions a fee of $0.50 is charged. For SEPA transactions a fee of $1 is charged.'
}
/>
</>
)}
{offrampType == _consts.OfframpType.CLAIM && (
<>
$0
<MoreInfo text={'Fees are on us, enjoy!'} />
</>
)}
${fee}
<MoreInfo
text={feeExplainer}
/>
</span>
</div>
<div className="flex w-full flex-row items-center justify-between gap-1 px-2 text-h8 text-gray-1">
Expand Down
2 changes: 1 addition & 1 deletion src/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export * from './general.utils'
export * from './sdkErrorHandler.utils'
export * from './fetch.utils'
export * from './cashout.utils'
export * from './offramp.utils'
22 changes: 22 additions & 0 deletions src/utils/cashout.utils.ts → src/utils/offramp.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as utils from '@/utils'
import countries from 'i18n-iso-countries'
import { generateKeysFromString } from '@squirrel-labs/peanut-sdk'
import { getSquidRouteRaw } from '@squirrel-labs/peanut-sdk'
import { achFeeExplainer, claimLinkFeeExplainer, OFFRAMP_IBAN_FEE_USD, OFFRAMP_NON_IBAN_FEE_USD, OfframpType, sepaFeeExplainer } from '@/components/Offramp/Offramp.consts'

export const convertPersonaUrl = (url: string) => {
const parsedUrl = new URL(url)
Expand Down Expand Up @@ -527,3 +528,24 @@ export const fetchRouteRaw = async (
return undefined
}
}

export function returnOfframpFee(offrampType: OfframpType, accountType: any): number {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

let fee = 0;
if (offrampType == OfframpType.CASHOUT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

fee = accountType === 'iban' ? OFFRAMP_IBAN_FEE_USD : OFFRAMP_NON_IBAN_FEE_USD
}
// other types of offramp (eg. CLAIM link) do not have a fee
return fee;
}

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
}
Comment on lines +541 to +551
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Uses a switch statement for clearer handling of different offramp types.
  2. Removes unnecessary variable assignment.
  3. Handles unrecognized offramp types with a warning.
  4. Is more concise and easier to extend in the future.