Skip to content

Fix/limit dollar min max checks on offramp claim link #420

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

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Fix/limit dollar min max checks on offramp claim link #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45d4d98
refactor: Move min, max cashout limits to Offramp const file
panosfilianos Oct 7, 2024
4c1e9d3
fix: Add check for dollar amount min, max on offramp link claims
panosfilianos Oct 7, 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
4 changes: 1 addition & 3 deletions src/components/Cashout/Components/Initial.view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { RecipientInfoComponent } from './RecipientInfo.comp'
import { motion, AnimatePresence } from 'framer-motion'
import Icon from '@/components/Global/Icon'
import { twMerge } from 'tailwind-merge'
import { MAX_CASHOUT_LIMIT, MIN_CASHOUT_LIMIT } from '@/components/Offramp/Offramp.consts'

export const InitialCashoutView = ({
onNext,
Expand Down Expand Up @@ -80,9 +81,6 @@ export const InitialCashoutView = ({
const [newBankAccount, setNewBankAccount] = useState<string>('')
const [activeInput, setActiveInput] = useState<'newBankAccount' | 'selectedBankAccount'>()

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

const isBelowMinLimit = useMemo(() => {
return !usdValue || parseFloat(usdValue) < MIN_CASHOUT_LIMIT
}, [usdValue])
Expand Down
34 changes: 32 additions & 2 deletions src/components/Claim/Link/Initial.view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { Popover } from '@headlessui/react'
import { useAuth } from '@/context/authContext'
import { ActionType, estimatePoints } from '@/components/utils/utils'
import { CrispButton } from '@/components/CrispChat'
import { optimismChainId, usdcAddressOptimism } from '@/components/Offramp/Offramp.consts'
import { MAX_CASHOUT_LIMIT, MIN_CASHOUT_LIMIT, optimismChainId, usdcAddressOptimism } from '@/components/Offramp/Offramp.consts'

export const InitialClaimLinkView = ({
onNext,
Expand Down Expand Up @@ -142,6 +142,22 @@ export const InitialClaimLinkView = ({
let tokenName = utils.getBridgeTokenName(claimLinkData.chainId, claimLinkData.tokenAddress)
let chainName = utils.getBridgeChainName(claimLinkData.chainId)

if (tokenPrice) {
const cashoutUSDAmount = Number(claimLinkData.tokenAmount) * tokenPrice
if (cashoutUSDAmount < MIN_CASHOUT_LIMIT) {
setErrorState({
showError: true,
errorMessage: 'offramp_lt_minimum',
})
return
} else if (cashoutUSDAmount > MAX_CASHOUT_LIMIT) {
setErrorState({
showError: true,
errorMessage: 'offramp_mt_maximum',
})
}
}
Comment on lines +145 to +159
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

Missing 'return' after setting error state for exceeding maximum cashout limit

When cashoutUSDAmount > MAX_CASHOUT_LIMIT, the error state is set but the function does not return. This may cause the function to continue executing, possibly leading to unintended behavior.

Apply this diff to fix the issue:

            } else if (cashoutUSDAmount > MAX_CASHOUT_LIMIT) {
                setErrorState({
                    showError: true,
                    errorMessage: 'offramp_mt_maximum',
                })
+               return
            }
📝 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
if (tokenPrice) {
const cashoutUSDAmount = Number(claimLinkData.tokenAmount) * tokenPrice
if (cashoutUSDAmount < MIN_CASHOUT_LIMIT) {
setErrorState({
showError: true,
errorMessage: 'offramp_lt_minimum',
})
return
} else if (cashoutUSDAmount > MAX_CASHOUT_LIMIT) {
setErrorState({
showError: true,
errorMessage: 'offramp_mt_maximum',
})
}
}
if (tokenPrice) {
const cashoutUSDAmount = Number(claimLinkData.tokenAmount) * tokenPrice
if (cashoutUSDAmount < MIN_CASHOUT_LIMIT) {
setErrorState({
showError: true,
errorMessage: 'offramp_lt_minimum',
})
return
} else if (cashoutUSDAmount > MAX_CASHOUT_LIMIT) {
setErrorState({
showError: true,
errorMessage: 'offramp_mt_maximum',
})
return
}
}


if (tokenName && chainName) {
} else {
if (!crossChainDetails) {
Expand Down Expand Up @@ -616,9 +632,9 @@ export const InitialClaimLinkView = ({
</label>
) : (
<>
<label className=" text-h8 font-normal text-red ">{errorState.errorMessage}</label>
{errorState.errorMessage === 'No route found for the given token pair.' && (
<>
<label className=" text-h8 font-normal text-red ">{errorState.errorMessage}</label>
{' '}
<span
className="cursor-pointer text-h8 font-normal text-red underline"
Expand All @@ -635,6 +651,20 @@ export const InitialClaimLinkView = ({
</span>
</>
)}
{errorState.errorMessage === 'offramp_lt_minimum' && (
<>
<label className=" text-h8 font-normal text-red ">
You can not claim links with less than ${MIN_CASHOUT_LIMIT} to your bank account.{' '}
</label>
</>
)}
{errorState.errorMessage === 'offramp_mt_maximum' && (
<>
<label className=" text-h8 font-normal text-red ">
You can not claim links with more than ${MAX_CASHOUT_LIMIT} to your bank account.{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny-nit: is this formatted as "$10100" ? not perfect but I VERY MUCH DOUBT anyone will ever run into this 😆

</label>
</>
)}
</>
)}
</div>
Expand Down
3 changes: 3 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,9 @@ export enum OfframpType {
CLAIM = 'CLAIM'
}

export const MIN_CASHOUT_LIMIT = 10 // $10 minimum
export const MAX_CASHOUT_LIMIT = 101000 // $101,000 maximum
Comment on lines +27 to +28
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing the constant declarations for improved maintainability and type safety.

The addition of these constants aligns well with the PR objectives and improves code maintainability. However, consider the following suggestions:

  1. Use descriptive variable names instead of magic numbers to improve readability:
const MIN_CASHOUT_DOLLARS = 10;
const MAX_CASHOUT_DOLLARS = 101000;

export const MIN_CASHOUT_LIMIT = MIN_CASHOUT_DOLLARS;
export const MAX_CASHOUT_LIMIT = MAX_CASHOUT_DOLLARS;

  1. Add a comment explaining the reasoning behind the specific maximum limit of $101,000.

  2. Use TypeScript's const assertions for added type safety:

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

These changes will further enhance the maintainability and clarity of the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. is fair

why min: else fees don't make sense (~$1 fee for each cashout roughly)
why max: just to be safe, we can consider increasing as time goes on. Unlikely to be exceeded

Copy link
Contributor

Choose a reason for hiding this comment

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

@Hugo0, thank you for explaining the reasoning behind the minimum and maximum cashout limits. Adding these comments will improve code readability and assist future developers in understanding these constraints.

(❁´◡`❁)

✏️ Learnings added 
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#420
File: src/components/Offramp/Offramp.consts.ts:27-28
Timestamp: 2024-10-07T13:23:54.767Z
Learning: In `src/components/Offramp/Offramp.consts.ts`, the `MIN_CASHOUT_LIMIT` is set to $10 because smaller amounts are impractical due to approximately $1 fee per cashout.

Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#420
File: src/components/Offramp/Offramp.consts.ts:27-28
Timestamp: 2024-10-07T13:23:54.767Z
Learning: In `src/components/Offramp/Offramp.consts.ts`, the `MAX_CASHOUT_LIMIT` is set to $101,000 as a safety measure; it can be increased over time if needed, though it's unlikely to be exceeded.

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.


export const usdcAddressOptimism = '0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85'
export const optimismChainId = '10'

Expand Down