Fix/limit dollar min max checks on offramp claim link #420
Conversation
panosfilianos
commented
Oct 7, 2024
panosfilianos
commented
- Moves min, max to Offramp consts file
- Adds dollar min, max limits on offramp claim links
📝 WalkthroughWalkthroughThe changes in this pull request involve the introduction of two new constants, 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/components/Cashout/Components/Initial.view.tsx (2)
Line range hint
329-338: LGTM: Updated error messages with centralized constantsThe error messages for minimum and maximum cashout limits now correctly use the imported
MIN_CASHOUT_LIMITandMAX_CASHOUT_LIMITconstants. This ensures consistency between the displayed limits and the validation logic.For consistency, consider applying
toLocaleString()toMIN_CASHOUT_LIMITas well:- <ChakraIcon name="warning" className="-mt-0.5" /> Minimum cashout amount is ${MIN_CASHOUT_LIMIT}. + <ChakraIcon name="warning" className="-mt-0.5" /> Minimum cashout amount is ${MIN_CASHOUT_LIMIT.toLocaleString()}.
Line range hint
1-341: Overall: Changes align with PR objectives and improve code qualityThe modifications in this file successfully implement the use of centralized constants for cashout limits. These changes improve code maintainability and ensure consistency throughout the component. The core logic remains intact, and the updates align well with the PR objectives.
Consider creating a custom hook (e.g.,
useCashoutLimits) to encapsulate the limit validation logic. This would further improve code organization and reusability, especially if these checks are needed in other components.src/components/Claim/Link/Initial.view.tsx (2)
657-658: Correct grammatical error in error message for minimum cashout limitThe error message should use "cannot" instead of "can not" for standard grammar usage.
Apply this diff to fix the error message:
- You can not claim links with less than ${MIN_CASHOUT_LIMIT} to your bank account. + You cannot claim links with less than ${MIN_CASHOUT_LIMIT} to your bank account.
664-665: Correct grammatical error in error message for maximum cashout limitThe error message should use "cannot" instead of "can not" for standard grammar usage.
Apply this diff to fix the error message:
- You can not claim links with more than ${MAX_CASHOUT_LIMIT} to your bank account. + You cannot claim links with more than ${MAX_CASHOUT_LIMIT} to your bank account.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/Cashout/Components/Initial.view.tsx (1 hunks)
- src/components/Claim/Link/Initial.view.tsx (4 hunks)
- src/components/Offramp/Offramp.consts.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/components/Cashout/Components/Initial.view.tsx (2)
22-22: LGTM: Centralized constants importThe import of
MIN_CASHOUT_LIMITandMAX_CASHOUT_LIMITfrom a centralized constants file aligns with the PR objectives and improves code maintainability.
Line range hint
103-109: LGTM: Updated limit checks using centralized constantsThe
isBelowMinLimitandisExceedingMaxLimithooks now correctly use the importedMIN_CASHOUT_LIMITandMAX_CASHOUT_LIMITconstants. This change ensures consistency with the centralized limits and improves maintainability.src/components/Claim/Link/Initial.view.tsx (1)
23-23: Importing cashout limit constants and Optimism chain detailsThe import statement correctly includes the necessary constants for cashout limits and Optimism chain details.
| export const MIN_CASHOUT_LIMIT = 10 // $10 minimum | ||
| export const MAX_CASHOUT_LIMIT = 101000 // $101,000 maximum |
There was a problem hiding this comment.
🛠️ 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:
- 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;-
Add a comment explaining the reasoning behind the specific maximum limit of $101,000.
-
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 maximumThese changes will further enhance the maintainability and clarity of the code.
There was a problem hiding this comment.
- 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
There was a problem hiding this comment.
@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.
| 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', | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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 | |
| } | |
| } |
| {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.{' '} |
There was a problem hiding this comment.
tiny-nit: is this formatted as "$10100" ? not perfect but I VERY MUCH DOUBT anyone will ever run into this 😆
| export const MIN_CASHOUT_LIMIT = 10 // $10 minimum | ||
| export const MAX_CASHOUT_LIMIT = 101000 // $101,000 maximum |
There was a problem hiding this comment.
- 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
