[TASK-7301] Sync main into develop #553
Conversation
Bypass gas estimation for Safe wallet connector
…ccount [TASK-7285] fix(cashout): format bank account number
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications to the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Create/useCreateLink.tsx (2)
188-192: Consider more robust Safe connector detectionThe current implementation relies on a simple string check which could be fragile. Consider using a more robust method like checking connector capabilities or maintaining a list of known Safe connector identifiers.
-const isSafeConnector = (connector?: { name?: string }): boolean => { - const name = connector?.name - if (!name) return false - return name.toLowerCase().includes('safe') -} +const SAFE_CONNECTORS = ['safe', 'gnosis safe']; +const isSafeConnector = (connector?: { name?: string }): boolean => { + const name = connector?.name?.toLowerCase(); + if (!name) return false; + return SAFE_CONNECTORS.some(safeConnector => name.includes(safeConnector)); +};
194-206: TODO comment needs clarificationThe TODO comment mentions "internut" and AA (Account Abstraction) but lacks context. Consider documenting:
- Why Safe connectors need zero gas fees
- What "internut" refers to
- The timeline for implementing a proper abstraction
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Cashout/Components/Initial.view.tsx(2 hunks)src/components/Create/useCreateLink.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Cashout/Components/Initial.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#484
File: src/components/Cashout/Components/Initial.view.tsx:273-274
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In the `InitialCashoutView` component (`src/components/Cashout/Components/Initial.view.tsx`), linked bank accounts should not generate error states, and the `ValidatedInput` component will clear any error messages if needed. Therefore, it's unnecessary to manually clear the error state when selecting or clearing linked bank accounts.
🔇 Additional comments (1)
src/components/Create/useCreateLink.tsx (1)
194-206: Verify gas fee handling for Safe transactions
The zero gas fee assumption for Safe connectors might need additional validation:
- Safe transactions might still incur gas fees at the wallet level
- The balance check in
prepareCreateLinkWrappermight need to account for Safe-specific fee structures
Also applies to: 577-589
✅ Verification successful
Let me analyze the gas fee handling patterns further by checking for Safe-specific transaction handling and balance checks.
Zero gas fees for Safe transactions are correctly handled
The implementation is correct for the following reasons:
- The zero gas fee assumption for Safe connectors is intentional as indicated by the TODO comment mentioning it's a requirement for "internut" (which injects Account Abstraction with zero gas fees)
- The balance check logic in
checkUserHasEnoughBalancecorrectly handles both Safe and non-Safe cases:- For Safe transactions, the gas fees are 0, so only token value is checked
- For regular transactions, it includes gas fees in the balance check:
tokenValue: String(Number(tokenValue) + maxGasAmount)
- Error handling is properly implemented with appropriate fallbacks and user feedback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Safe-specific gas handling patterns in the codebase
rg -A 5 "gasLimit|maxFeePerGas|gasPrice" --type ts
Length of output: 5006
Script:
#!/bin/bash
# Check for Safe-specific transaction handling and balance checks
rg -A 10 "isSafeConnector|checkUserHasEnoughBalance" --type ts
Length of output: 12081
| autoComplete="on" | ||
| name="bank-account" | ||
| formatDisplayValue={formatBankAccountDisplay} | ||
| formatDisplayValue={(value) => formatBankAccountDisplay(value, 'iban')} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider dynamic account type for formatBankAccountDisplay
The hardcoded 'iban' type parameter might cause formatting issues for US bank accounts. Consider deriving the account type from the input format or validation result.
-formatDisplayValue={(value) => formatBankAccountDisplay(value, 'iban')}
+formatDisplayValue={(value) => {
+ const isUSAccount = value.length > 9 && /^\d+$/.test(value);
+ return formatBankAccountDisplay(value, isUSAccount ? 'us' : 'iban');
+}}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation