Feat/move liquidation address earlier #458
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across three primary components: Changes
Possibly related PRs
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: 10
🧹 Outside diff range and nitpick comments (1)
src/utils/cashout.utils.ts (1)
530-530: Approved: Improved flexibility, but consider refining the placeholder address usageThe changes to
fetchRouteRawfunction improve its flexibility by makingfromAddressoptional. This aligns well with the PR objective of moving liquidation address creation earlier in the process.However, using a hardcoded placeholder address when
fromAddressis not provided might not be suitable for all scenarios. Consider the following suggestions:
- Use a more generic placeholder address (e.g., zero address) or make it configurable.
- Add a comment explaining why this placeholder is used and its implications.
Here's a potential improvement:
- fromAddress: fromAddress ?? '0x9647BB6a598c2675310c512e0566B60a5aEE6261', // placeholder address just to get a route sample + fromAddress: fromAddress ?? '0x0000000000000000000000000000000000000000', // Zero address as placeholder to get a route sampleAlso, consider adding a comment explaining the use of the placeholder:
// Note: We use a placeholder address when fromAddress is not provided to get a route sample. // This is acceptable for route estimation but should be replaced with the actual address for the final transaction.Also applies to: 540-541
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/Claim/Link/Initial.view.tsx (4 hunks)
- src/components/Offramp/Confirm.view.tsx (8 hunks)
- src/utils/cashout.utils.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/utils/cashout.utils.ts (2)
210-210: LGTM: Minor improvements to return statementsThe changes to the return statements in the
createExternalAccountfunction are minor but positive. They improve clarity by explicitly including thedatain the error case and slightly reformatting the success case. These changes maintain the function's behavior while enhancing readability.Also applies to: 218-218
Line range hint
1-551: Overall assessment: Changes align with PR objectives and improve code flexibilityThe modifications in this file, particularly in the
createExternalAccountandfetchRouteRawfunctions, support the PR's goal of moving liquidation address creation earlier in the process. The changes improve code flexibility and clarity without introducing any significant issues.Key points:
- The
createExternalAccountfunction now has clearer return statements.- The
fetchRouteRawfunction is more flexible with the optionalfromAddressparameter.While the changes are generally positive, consider refining the use of the placeholder address in
fetchRouteRawas suggested in the previous comment.src/components/Claim/Link/Initial.view.tsx (1)
23-28: Imports are appropriately organized and added.The constants
MAX_CASHOUT_LIMIT,MIN_CASHOUT_LIMIT,optimismChainId, andusdcAddressOptimismare correctly imported from'@/components/Offramp/Offramp.consts'for use within the component.
| Number(claimLinkData.tokenAmount) * Math.pow(10, claimLinkData.tokenDecimals) | ||
| ).toString() | ||
|
|
||
| // TODO: this is duplicate with src/utils/fetchRouteRaw |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated route fetching code.
The comment indicates that the code for fetching routes duplicates logic from src/utils/fetchRouteRaw. To improve maintainability and reduce code duplication, consider refactoring this section to use the existing utility function.
Would you like assistance in refactoring this code to use fetchRouteRaw?
| <label className=" text-h8 font-normal text-red "> | ||
| {errorState.errorMessage} | ||
| </label>{' '} |
There was a problem hiding this comment.
Potential XSS vulnerability when rendering error messages.
Directly rendering errorState.errorMessage could expose the application to cross-site scripting (XSS) attacks if the error message contains untrusted input. Ensure that errorState.errorMessage is properly sanitized or originates from a trusted source before rendering.
There was a problem hiding this comment.
gigabrain vulnerability injection pattern
There was a problem hiding this comment.
@panosfilianos, understood. If there's anything else I can assist with, please let me know.
(？･ｪ･？)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| You can not claim links with more than ${MAX_CASHOUT_LIMIT} to your bank | ||
| account.{' '} |
There was a problem hiding this comment.
Fix variable interpolation in error message for maximum limit.
The variable MAX_CASHOUT_LIMIT will not be interpolated correctly inside the string. In JSX, you need to wrap the variable inside curly braces {} to interpolate it.
Apply this diff to fix the interpolation:
-<label className=" text-h8 font-normal text-red ">
- You can not claim links with more than ${MAX_CASHOUT_LIMIT} to your bank
+</label>
+<label className=" text-h8 font-normal text-red ">
+ {`You can not claim links with more than $${MAX_CASHOUT_LIMIT} to your bank account.`}
+</label>Committable suggestion was skipped due to low confidence.
| You can not claim links with less than ${MIN_CASHOUT_LIMIT} to your bank | ||
| account.{' '} |
There was a problem hiding this comment.
Fix variable interpolation in error message for minimum limit.
The variable MIN_CASHOUT_LIMIT will not be interpolated correctly inside the string. In JSX, you need to wrap the variable inside curly braces {} to interpolate it.
Apply this diff to fix the interpolation:
-<label className=" text-h8 font-normal text-red ">
- You can not claim links with less than ${MIN_CASHOUT_LIMIT} to your bank
+</label>
+<label className=" text-h8 font-normal text-red ">
+ {`You can not claim links with less than $${MIN_CASHOUT_LIMIT} to your bank account.`}
+</label>Committable suggestion was skipped due to low confidence.
| // functions for claim link offramps (not self-cashout) | ||
| // TODO: they need to be refactored to a separate file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor functions into a separate file for better modularity
The TODO comment indicates that the functions for claim link offramps need to be refactored into a separate file. Moving these functions to a dedicated file will improve code organization, reusability, and maintainability.
Consider creating a new file (e.g., claimLinkOfframpUtils.ts) and move all related functions there.
| // functions for cashout offramps | ||
| // TODO: they need to be refactored to a separate file | ||
|
|
||
| // TODO: this function is a clusterfuck |
There was a problem hiding this comment.
Avoid unprofessional language in comments
The comment at line 96 uses unprofessional language ("this function is a clusterfuck"). It's important to maintain professional language in code comments. Please rephrase the comment to clearly describe the issue in a professional manner.
Apply this diff to correct the comment:
- // TODO: this function is a clusterfuck
+ // TODO: Refactor this function to improve readability and maintainability📝 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.
| // TODO: this function is a clusterfuck | |
| // TODO: Refactor this function to improve readability and maintainability |
There was a problem hiding this comment.
@Hugo0, I'm sorry to hear that you're feeling this way. If there's anything I can assist you with, please let me know.
(•́_•̀)
There was a problem hiding this comment.
bullying bots in not a crime cr (yet)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
btw we do need to refactor this function asap
There was a problem hiding this comment.
linus is always crass and hes great @coderabbitai
youre wrong
There was a problem hiding this comment.
@Hugo0, understood. I'll keep that in mind for future reviews.
(｡•́︿•̀｡)
✏️ Learnings added
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#458
File: src/components/Offramp/Confirm.view.tsx:96-96
Timestamp: 2024-10-18T08:54:21.961Z
Learning: In the `src/components/Offramp/Confirm.view.tsx` file, it's acceptable to include crass or informal language in code comments.
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.
| // TODO: type safety | ||
| const { tokenName, chainName, xchainNeeded, liquidationAddress } = await processLinkDetails( | ||
| claimLinkData, | ||
| preparedCreateLinkWrapperResponse.linkDetails, |
There was a problem hiding this comment.
Fix type safety by replacing 'any' types with proper interfaces
The parameter claimLinkData is currently typed as any in the processLinkDetails function, and there's a TODO comment to fix the type. Using any undermines TypeScript's type system. Please define appropriate interfaces or types for claimLinkData and other parameters to ensure type safety and catch potential errors at compile time.
Apply this diff to update the function signature:
- const processLinkDetails = async (
- claimLinkData: any, // TODO: fix type
+ import { ClaimLinkData } from '@/types' // Assuming you have a types file
+ const processLinkDetails = async (
+ claimLinkData: ClaimLinkData,Similarly, update the handleCrossChainScenario function:
- const handleCrossChainScenario = async (claimLinkData: any, crossChainDetails: CrossChainDetails[]) => {
+ const handleCrossChainScenario = async (
+ claimLinkData: ClaimLinkData,
+ crossChainDetails: CrossChainDetails[]
+ ) => {Ensure that the ClaimLinkData interface accurately represents the structure of claimLinkData.
Committable suggestion was skipped due to low confidence.
| claimLinkData.tokenAddress!, | ||
| claimLinkData.chainId!.toString(), | ||
| usdcAddressOptimism, | ||
| optimismChainId, | ||
| claimLinkData.tokenDecimals, | ||
| claimLinkData.tokenAmount, | ||
| claimLinkData.senderAddress | ||
| claimLinkData.tokenDecimals!, | ||
| claimLinkData.tokenAmount!, | ||
| claimLinkData.senderAddress ?? '0x9647BB6a598c2675310c512e0566B60a5aEE6261' |
There was a problem hiding this comment.
Avoid overusing the non-null assertion operator '!'
In lines 282-288, the properties of claimLinkData are accessed using the non-null assertion operator !. Overusing ! can suppress potential null or undefined errors and may lead to runtime exceptions if the values are indeed null or undefined. Consider adding proper null checks or default values, or ensure that claimLinkData is correctly typed to prevent the need for non-null assertions.
Apply this diff to add proper null checks and improve type safety:
- claimLinkData.tokenAddress!,
- claimLinkData.chainId!.toString(),
+ if (!claimLinkData.tokenAddress || !claimLinkData.chainId) {
+ throw new Error('Missing tokenAddress or chainId in claimLinkData');
+ }
+ claimLinkData.tokenAddress,
+ claimLinkData.chainId.toString(),
...
- claimLinkData.tokenDecimals!,
- claimLinkData.tokenAmount!,
+ if (!claimLinkData.tokenDecimals || !claimLinkData.tokenAmount) {
+ throw new Error('Missing tokenDecimals or tokenAmount in claimLinkData');
+ }
+ claimLinkData.tokenDecimals,
+ claimLinkData.tokenAmount,
...
- claimLinkData.senderAddress ?? '0x9647BB6a598c2675310c512e0566B60a5aEE6261'
+ claimLinkData.senderAddress || '0x9647BB6a598c2675310c512e0566B60a5aEE6261'Alternatively, ensure that claimLinkData is properly typed and its properties are non-nullable.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/components/utils/utils.ts (3)
79-79: Approved: Logging severity adjustment improves error handling.The change from
console.errortoconsole.warnis appropriate for this context. It aligns with the PR objectives by potentially reducing unnecessary alarm for transient issues during transaction status checks.Consider adding a more descriptive error message to provide context about the specific operation that failed. For example:
console.warn('Error fetching transaction status from Squid Router API:', error)This will make it easier to identify the source of the warning in logs, especially if this utility is used in multiple places.
Line range hint
84-106: Consider adding a timeout mechanism tofetchDestinationChain.The current implementation of
fetchDestinationChainusessetIntervalto repeatedly check the transaction status. However, there's no mechanism to stop this process if the transaction never reaches a 'success' state, which could lead to unnecessary API calls and potential resource waste.Consider implementing a timeout mechanism. Here's a suggested approach:
export async function fetchDestinationChain( txHash: string, setExplorerUrlDestChainWithTxHash: (value: { transactionId: string; transactionUrl: string }) => void, maxAttempts = 60, // 5 minutes at 5-second intervals intervalMs = 5000 ) { let attempts = 0; let intervalId = setInterval(async () => { if (attempts >= maxAttempts) { clearInterval(intervalId); console.warn(`Transaction ${txHash} did not succeed after ${maxAttempts} attempts.`); return; } attempts++; const result = await checkTransactionStatus(txHash); // ... rest of the function remains the same }, intervalMs); }This modification ensures that the function will stop checking after a certain number of attempts, preventing indefinite API calls for transactions that may never succeed.
Line range hint
114-156: Consider externalizing the API URL inestimatePoints.The
estimatePointsfunction currently uses a hardcoded API URL (consts.PEANUT_API_URL). While this isn't directly related to the changes in this PR, it's a potential area for improvement.Consider moving the API URL to an environment variable or a configuration file. This would make it easier to change the URL for different environments (development, staging, production) without modifying the code.
Example:
const API_URL = process.env.PEANUT_API_URL || consts.PEANUT_API_URL; export const estimatePoints = async ({ // ... function parameters remain the same }) => { try { const response = await fetch(`${API_URL}/calculate-pts-for-action`, { // ... rest of the function remains the same }); // ... rest of the function remains the same } catch (error) { console.error('Failed to estimate points:', error); return 0; } }This change would improve the flexibility and maintainability of the code.
src/utils/cashout.utils.ts (1)
Line range hint
197-210: Define a specific error type for duplicate external accountsIn the error handling for duplicate external accounts, the code returns a response with
success: falseand adatafield that contains an undefined error type. This can lead to ambiguity and type safety issues. Consider defining a specific interface for the error response to standardize the structure and improve maintainability.For example, you can define an interface like:
interface IDuplicateExternalAccountError { id: string; code: string; message: string; }Then update the return statement:
return { success: false, - data, + data: data as IDuplicateExternalAccountError, } as interfaces.IResponse;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/Offramp/Confirm.view.tsx (9 hunks)
- src/components/utils/utils.ts (1 hunks)
- src/utils/cashout.utils.ts (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Offramp/Confirm.view.tsx
🧰 Additional context used
🔇 Additional comments (2)
src/utils/cashout.utils.ts (2)
218-218: Confirm successful account creation return typeThe return statement correctly casts
datatointerfaces.IBridgeAccount, ensuring type consistency and type safety in the response when the external account creation is successful.
484-484: Add description for 'AWAITING_FIAT' statusThe
CashoutStatusDescriptionsobject now includes a description for the new'AWAITING_FIAT'status. This ensures that users will receive appropriate messages corresponding to this status.
| tokenDecimals: number, | ||
| tokenAmount: string, | ||
| senderAddress: string | ||
| fromAddress?: string |
There was a problem hiding this comment.
Avoid hardcoding placeholder addresses in function parameters
In the fetchRouteRaw function, fromAddress is optional and defaults to a placeholder address if not provided. Similarly, toAddress is set to a hardcoded placeholder. Using hardcoded addresses can lead to unintended behavior and potential security risks in production. It's better to require these addresses as parameters or handle the absence more explicitly.
Consider updating the function to require fromAddress and toAddress as parameters:
export const fetchRouteRaw = async (
fromToken: string,
fromChain: string,
toToken: string,
toChain: string,
tokenDecimals: number,
tokenAmount: string,
- fromAddress?: string
+ fromAddress: string,
+ toAddress: string
) => {
try {
const _tokenAmount = BigInt(Math.floor(Number(tokenAmount) * Math.pow(10, tokenDecimals))).toString();
const route = await getSquidRouteRaw({
squidRouterUrl: 'https://apiplus.squidrouter.com/v2/route',
fromChain: fromChain,
fromToken: fromToken.toLowerCase(),
fromAmount: _tokenAmount,
- fromAddress: fromAddress ?? '0x9647BB6a598c2675310c512e0566B60a5aEE6261', // placeholder address just to get a route sample
- toAddress: '0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45C', // placeholder address just to get a route sample
+ fromAddress: fromAddress,
+ toAddress: toAddress,
toChain: toChain,
toToken: toToken,
slippage: 1,
});
return route;
} catch (error) {
console.error('Error fetching route:', error);
return undefined;
}
};This change enforces explicit handling of addresses and reduces the risk of accidental use of placeholder values.
Also applies to: 542-543
| | 'REFUNDED' | ||
| | 'READY' | ||
| | 'AWAITING_TX' | ||
| | 'AWAITING_FIAT' |
There was a problem hiding this comment.
💡 Codebase verification
'AWAITING_FIAT' Status Not Fully Handled
The new AWAITING_FIAT status is defined in src/utils/cashout.utils.ts but does not appear to be handled in any conditional logic or processing within the application. Please ensure that all relevant parts of the codebase account for and properly handle the AWAITING_FIAT status to prevent potential issues.
- File:
src/utils/cashout.utils.ts - Line: 451
🔗 Analysis chain
Ensure new 'AWAITING_FIAT' status is handled appropriately
A new CashoutStatus value 'AWAITING_FIAT' has been added. Please verify that all parts of the application that handle CashoutStatus are updated to accommodate this new status to prevent any potential issues.
You can run the following script to check where CashoutStatus is used and ensure 'AWAITING_FIAT' is handled:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of 'CashoutStatus' and check for handling of 'AWAITING_FIAT'
rg --type typescript 'CashoutStatus'
Length of output: 71
Script:
#!/bin/bash
# Description: Find all usages of 'CashoutStatus' in TypeScript files and verify handling of 'AWAITING_FIAT'
# Search for 'CashoutStatus' in .ts and .tsx files
rg -g "*.ts" -g "*.tsx" 'CashoutStatus' > cashout_status_usages.txt
# Check if 'AWAITING_FIAT' is handled in the found usages
rg -g "*.ts" -g "*.tsx" "'AWAITING_FIAT'" cashout_status_usages.txt
Length of output: 115
Script:
#!/bin/bash
# Description: Search for 'AWAITING_FIAT' across all TypeScript files to verify its handling
rg -g "*.ts" -g "*.tsx" "'AWAITING_FIAT'"
Length of output: 98
| claimLinkData.senderAddress | ||
| claimLinkData.tokenDecimals!, | ||
| claimLinkData.tokenAmount!, | ||
| claimLinkData.senderAddress ?? '0x9647BB6a598c2675310c512e0566B60a5aEE6261' |
There was a problem hiding this comment.
issue (non-blocking): can we abstract this to a consts file somehow? It's unclear what this address does here.
There was a problem hiding this comment.
yep this is bad and even superfluous because i alrdy added it to utils fetch func:
Purpose is to get x-chain route even before all details are known
| tokenAddress: string | ||
| ) => { | ||
| tokenAddress: string, | ||
| isSameChain?: boolean // e.g. for opt ETH -> opt USDC |
There was a problem hiding this comment.
thought: this likely pertains to the hardcoded addy above. We need to make different functions for unique flow behavior and abstrac these custom rules there imo
There was a problem hiding this comment.
No - this is because of PR 457 https://github.com/peanutprotocol/peanut-ui/pull/457/files
when same chain but still token swap, the destination chain hash is same as source chain hash, and has to be handled differently
| if (attempts < maxAttempts) { | ||
| await new Promise((resolve) => setTimeout(resolve, 2000)) | ||
| } |
There was a problem hiding this comment.
issue (non-blocking): I'd comment why we need this
| fromAddress: fromAddress ?? '0x9647BB6a598c2675310c512e0566B60a5aEE6261', // placeholder address just to get a route sample | ||
| toAddress: '0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45C', // placeholder address just to get a route sample |
There was a problem hiding this comment.
nit: Ideally the sample route flow fetching would be default values on the func that provides the route or the addresses part of consts file
|
|
||
| const srcChainId = claimLinkData.chainId | ||
| const destChainId = chainId | ||
| const isSameChain = srcChainId === destChainId |
There was a problem hiding this comment.
issue (non-blocking): there's behavior differentiation in theis func based on this var. It's not immediatelly clear the impact of it. I'd add 2 lines of a comment here explain that.
Problem: liquidation address creation could fail. Sucks if user pays and THEN gets error.
Moved logic earlier.
@panosfilianos the type safety is a mess here - please take a look and lmk if this refactor seems reasonable to you
EDIT: did some debugging and QA and seems fine now