[TASK-7887] fix: remove lopping for status check in cross-chain cashout #602
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant OfframpConfirmView
participant ClaimLink
participant TransactionProcessor
User->>OfframpConfirmView: Initiate Transfer
OfframpConfirmView->>ClaimLink: Fetch Cross-Chain Details
ClaimLink-->>OfframpConfirmView: Return Details
OfframpConfirmView->>TransactionProcessor: Submit Transfer
TransactionProcessor-->>OfframpConfirmView: Process Result
OfframpConfirmView->>User: Display Transaction Status
Possibly related PRs
Poem
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: 0
🧹 Nitpick comments (4)
src/components/Offramp/Confirm.view.tsx (4)
7-8: Check for potential naming inconsistency.
import { GlobaLinkAccountComponent } …may indicate a naming or typographical mismatch from the intended “GlobalLinkAccountComponent.” Confirm that the names align for clarity and maintainability.
11-12: Broad imports usage.
Importing everything (* as consts,* as context) can reduce clarity on where symbols originate. Consider importing selectively if the modules are large or if tree-shaking is an important concern.
370-394: Consider removing commented-out polling logic.
Since you’ve decided to switch to a single transaction check, the old polling loop is merely commented out. If it’s no longer needed, removing it can keep the codebase clean.- // const maxAttempts = 15 - // let attempts = 0 - // while (attempts < maxAttempts) { - // try { ... } - // } - // console.warn('Transaction status check timed out...')
395-405: Single-attempt strategy may risk incomplete status.
Reliance on a singlecheckTransactionStatuscall can lead to using the source hash if the destination transaction details haven’t propagated yet. Consider adding optional retry logic or user feedback (e.g., a “Refresh Status” button) to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Offramp/Confirm.view.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Offramp/Confirm.view.tsx (1)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#458
File: src/components/Offramp/Confirm.view.tsx:141-141
Timestamp: 2024-11-12T09:39:20.720Z
Learning: The `handleConfirm` function in `src/components/Create/Link/Confirm.view.tsx` is separate from the one in `src/components/Offramp/Confirm.view.tsx` and does not need to be renamed when refactoring `handleConfirm` in `src/components/Offramp/Confirm.view.tsx`.
🔇 Additional comments (5)
src/components/Offramp/Confirm.view.tsx (5)
2-5: Consolidate or validate new imports.
Imports forsortCrossChainDetails,useClaimLink,useCreateLink, andCrispButtonhave been introduced. Verify they are all used consistently within the file to avoid unnecessary dependencies or confusion.
15-16: Validate utility function imports.
EnsureformatBankAccountDisplayandgetSquidTokenAddressare correctly and safely handling edge cases and invalid inputs, given their potentially user-facing scope.
18-20: Confirm external hook usage.
useStepsfromchakra-ui-stepsand standard React hooks are introduced. Confirm that these third-party and built-in hooks are compatible with the rest of your components’ lifecycle and state management.
32-32: Confirm existence and secure handling of the imported utility.
checkTransactionStatusplays a critical role in bridging logic; ensure it gracefully handles network or API failures without leaving the system in an uncertain state.
408-408: Fallback logic clarity.
The fallback to usesourceTxHashfor thedestinationTxHashcan lead to inaccurate final reporting if the cross-chain transaction is still initializing. Make sure to visibly inform users that a final transaction hash might update or confirm later.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Offramp/Confirm.view.tsx (1)
Line range hint
541-650: Refactor to reduce code duplicationThe
handleSubmitTransferfunction shares significant logic withhandleCashoutConfirm. Consider extracting common functionality into shared utility functions.Key areas to extract:
- Token and chain name resolution
- Liquidation address handling
- Transaction submission logic
Example refactor:
const resolveBridgeDetails = async (chainId: string, tokenAddress: string) => { const tokenName = utils.getBridgeTokenName(chainId, tokenAddress); const chainName = utils.getBridgeChainName(chainId); return { tokenName, chainName }; }; const getLiquidationAddress = async ( bridgeCustomerId: string, chainName: string, tokenName: string, bridgeExternalAccountId: string, recipientType: string ) => { // ... existing liquidation address logic };
🧹 Nitpick comments (3)
src/components/Offramp/Confirm.view.tsx (3)
416-420: Enhance error handling for claim operationsThe current implementation doesn't distinguish between different types of claim failures. Consider adding specific error messages for common failure scenarios (e.g., insufficient funds, network issues).
const sourceTxHash = await claimLink({ address, link: claimLinkData.link, -}) +}).catch(error => { + const errorMessage = error.message || 'Unknown error occurred'; + throw new Error(`Claim failed: ${errorMessage}`); +});
Line range hint
509-538: Add TypeScript types for cross-chain detailsThe function uses
anytype for chain details which could lead to runtime errors. Consider defining proper interfaces.+interface CrossChainOption { + chainId: string; + // add other required fields +} const getCrossChainDetails = async ({ chainId, tokenType, contractVersion, }: { chainId: string tokenType: number contractVersion: string -}) => { +}): Promise<CrossChainOption[] | undefined> => {
Line range hint
651-950: Improve component structure and readabilityThe render logic contains complex nested conditions and calculations. Consider:
- Extracting fee calculation logic into a separate component
- Moving complex conditional renders into separate components
- Simplifying nested ternary operators
Example component extraction:
const FeeDisplay = ({ appliedPromoCode, accountType }: { appliedPromoCode: string | null, accountType: string }) => { // Fee display logic }; const TotalAmountDisplay = ({ offrampType, usdValue, tokenPrice, claimLinkData }: { offrampType: OfframpType, usdValue?: string, tokenPrice?: number, claimLinkData?: any }) => { // Total amount display logic };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Offramp/Confirm.view.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Offramp/Confirm.view.tsx (1)
Learnt from: Hugo0
PR: peanutprotocol/peanut-ui#458
File: src/components/Offramp/Confirm.view.tsx:141-141
Timestamp: 2024-11-12T09:39:20.720Z
Learning: The `handleConfirm` function in `src/components/Create/Link/Confirm.view.tsx` is separate from the one in `src/components/Offramp/Confirm.view.tsx` and does not need to be renamed when refactoring `handleConfirm` in `src/components/Offramp/Confirm.view.tsx`.
🔇 Additional comments (1)
src/components/Offramp/Confirm.view.tsx (1)
370-409:⚠️ Potential issueCritical: Verify the impact of removing transaction status checks
The removal of transaction status polling could lead to potential issues:
- Users might see success screens before transactions are actually completed
- Failed cross-chain transactions might go undetected
- Backend might not have transaction data when needed
Let's verify the backend's capability to handle this change:
Summary by CodeRabbit
New Features
Refactor