[TASK-7159] [TASK-7192] [TASK-7193] feat: send money with peanut-wallet #550
Conversation
Next was complaining about missing packages
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request updates the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
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 (
|
jjramirezn
changed the title
jjramirezn
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Outside diff range and nitpick comments (5)
src/context/walletContext/walletContext.tsx (2)
71-79: Optimize by caching the default wallet instanceCurrently,
createDefaultDBWallet(wagmiAddress)is called twice, which is redundant. Caching the result in a variable can improve efficiency.Apply this diff to cache the default wallet:
return wagmiAddress ? [ + (() => { + const defaultWallet = createDefaultDBWallet(wagmiAddress); return { - ...createDefaultDBWallet(wagmiAddress), + ...defaultWallet, - connected: isWalletConnected(createDefaultDBWallet(wagmiAddress)), + connected: isWalletConnected(defaultWallet), balance: BigInt(0), }; + })(), ] : []
119-120: UseareAddressesEqualfor address comparisonsDirect string comparison of Ethereum addresses can lead to issues due to case sensitivity. Using
areAddressesEqualensures accurate, case-insensitive comparisons.For line 119:
- const wallet = wallets.find((w) => w.address === selectedAddress) + const wallet = wallets.find((w) => areAddressesEqual(w.address, selectedAddress))For line 140:
- const walletExists = wallets.some((wallet) => wallet.address === wagmiAddress) + const walletExists = wallets.some((wallet) => areAddressesEqual(wallet.address, wagmiAddress))Also applies to: 140-141
package.json (1)
58-58: Movepino-prettytodevDependencies
pino-prettyis primarily used for development logging. Placing it indevDependenciesprevents it from being included in the production build, reducing bundle size.Apply this diff to move
pino-pretty:"dependencies": { // other dependencies - "pino-pretty": "^13.0.0", }, "devDependencies": { // other devDependencies + "pino-pretty": "^13.0.0", },src/components/Request/Pay/Views/Success.view.tsx (1)
Line range hint
3-3: Consider adding tests for address comparison changesThe switch from
areTokenAddressesEqualtoareAddressesEqualspans across UI components and core functionality. Consider:
- Adding unit tests specifically for cross-chain scenarios
- Documenting the address comparison behavior in the codebase
- Adding integration tests for the payment processing flow
Also applies to: 14-14, 35-35, 56-56, 180-180
src/components/Claim/Link/Initial.view.tsx (1)
334-334: Consider extracting the hardcoded addressWhile the function rename is correct, consider extracting the hardcoded address
0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45Cused in the route finding logic into a named constant.+ const DEFAULT_ROUTE_ADDRESS = '0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45C' ... - ? '0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45C' - : recipient.address || '0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45C', + ? DEFAULT_ROUTE_ADDRESS + : recipient.address || DEFAULT_ROUTE_ADDRESS,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
package.json(1 hunks)src/components/Claim/Link/Initial.view.tsx(3 hunks)src/components/Create/Create.utils.ts(2 hunks)src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Global/ImageGeneration/LinkPreview.tsx(1 hunks)src/components/Global/TokenSelector/TokenSelector.tsx(5 hunks)src/components/Profile/index.tsx(1 hunks)src/components/Request/Pay/Views/Initial.view.tsx(3 hunks)src/components/Request/Pay/Views/Success.view.tsx(1 hunks)src/context/walletContext/walletContext.tsx(3 hunks)src/hooks/useBalance.tsx(3 hunks)src/utils/cashout.utils.ts(1 hunks)src/utils/general.utils.ts(3 hunks)src/utils/token.utils.ts(2 hunks)
🧰 Additional context used
📓 Learnings (3)
src/components/Global/ImageGeneration/LinkPreview.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#460
File: src/components/Global/ImageGeneration/LinkPreview.tsx:125-128
Timestamp: 2024-11-12T09:39:20.720Z
Learning: 'tokenPrice' is not used currently and will be removed in future PRs.
src/components/Request/Pay/Views/Success.view.tsx (1)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#409
File: src/components/Request/Pay/Views/Success.view.tsx:23-23
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In the `SuccessView` component, `transactionHash` remains constant because we are in a success view, so it's unnecessary to include it in the dependency array of `useEffect` hooks.
src/components/Request/Pay/Views/Initial.view.tsx (3)
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:67-74
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, the functions `getDefaultProvider` and `getTokenContractDetails` used in `fetchTokenSymbol` do not throw errors but can return `undefined`. Additional error handling is unnecessary since this is already handled in the code.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#410
File: src/components/Request/Pay/Views/Initial.view.tsx:87-93
Timestamp: 2024-11-12T09:39:20.720Z
Learning: When refactoring to eliminate code duplication, prioritize readability and consider whether the change significantly improves the code. If it doesn't enhance readability or maintainability, it's acceptable to keep the existing code structure.
Learnt from: jjramirezn
PR: peanutprotocol/peanut-ui#422
File: src/components/Request/Pay/Views/Initial.view.tsx:76-78
Timestamp: 2024-11-12T09:39:20.720Z
Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, both `txFee` and `utils.formatTokenAmount(estimatedGasCost, 3)` return strings, ensuring consistent return types for `calculatedFee`.
🔇 Additional comments (16)
src/utils/cashout.utils.ts (1)
444-444: LGTM
The change to use utils.areAddressesEqual improves address comparison accuracy.
src/utils/token.utils.ts (1)
1-1: LGTM
Replacing areTokenAddressesEqual with areAddressesEqual ensures consistent and accurate address comparisons.
Also applies to: 12-12, 19-19
src/components/Create/Create.utils.ts (1)
5-5: LGTM
Updating to areAddressesEqual enhances the reliability of address comparisons throughout the utility functions.
Also applies to: 54-54, 58-58, 64-64
src/components/Global/ImageGeneration/LinkPreview.tsx (1)
35-35: LGTM: Address comparison method updated
The change from areTokenAddressesEqual to areAddressesEqual is appropriate here as the function is used for general address comparison to find token logos.
src/hooks/useBalance.tsx (2)
14-14: Good: Simplified wallet context usage
The destructuring has been simplified to use just address and isConnected, making the code cleaner and more maintainable.
180-180: LGTM: Updated address comparison in balance lookup
The change from areTokenAddressesEqual to areAddressesEqual maintains consistency with the codebase-wide update while preserving the balance lookup functionality.
src/components/Request/Pay/Views/Success.view.tsx (1)
56-56: Verify payment processing with new address comparison
While the change from areTokenAddressesEqual to areAddressesEqual aligns with the codebase update, let's verify it doesn't affect the payment processing logic.
✅ Verification successful
Address comparison change is safe and consistent
The verification shows that areAddressesEqual is used consistently throughout the codebase for token address comparisons:
- It's the standard utility function for address comparison in both payment and claim flows
- The implementation in
general.utils.tsproperly handles native token address normalization - The function is used in similar token address comparison contexts across multiple components
The change from areTokenAddressesEqual to areAddressesEqual aligns with the codebase's standard practice and maintains the same payment processing logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other payment processing flows that might be affected
rg -A 5 "submitRequestLinkFulfillment" --type tsx --type ts
# Check for any test cases covering this scenario
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | xargs rg "areAddressesEqual.*tokenAddress"
Length of output: 183
Script:
#!/bin/bash
# Search for other payment processing flows that might be affected
rg -A 5 "submitRequestLinkFulfillment"
# Check for usage of areAddressesEqual with token addresses
rg -A 2 "areAddressesEqual.*token"
# Look for test files related to payment processing
fd "pay|payment" -e test.tsx -e test.ts -e spec.tsx -e spec.ts
# Check the utils file for areAddressesEqual implementation
rg -A 3 "areAddressesEqual.*="
Length of output: 8035
src/components/Profile/index.tsx (1)
190-190: LGTM! Function rename improves semantic clarity.
The change from areTokenAddressesEqual to areAddressesEqual better reflects that the function compares wallet addresses in the referral system, not specifically token addresses.
src/components/Create/Link/Confirm.view.tsx (1)
8-8: LGTM! Consistent address comparison in token lookup.
The change from areTokenAddressesEqual to areAddressesEqual maintains the same functionality while providing better semantic clarity. The function is correctly used to find tokens by their addresses in both Squid and Peanut supported chains.
Also applies to: 100-108
src/components/Global/TokenSelector/TokenSelector.tsx (1)
10-10: LGTM! Consistent address comparison across token selection logic.
The change from areTokenAddressesEqual to areAddressesEqual is consistently applied across all token comparison scenarios:
- Token selection highlighting
- Duplicate token filtering
- Balance matching
- Selected token lookup
The functionality remains the same while the naming better reflects the general-purpose address comparison.
Also applies to: 75-77, 197-197, 225-225, 233-233, 281-281
src/components/Request/Pay/Views/Initial.view.tsx (2)
210-210: LGTM: Consistent function usage
The function rename is consistently applied in the token logo lookup logic.
201-201: Function rename improves code clarity
The change from areTokenAddressesEqual to areAddressesEqual better reflects the function's general-purpose address comparison capability.
✅ Verification successful
Function rename is consistent across the codebase
The verification confirms that areAddressesEqual is the only version present in the codebase, defined in src/utils/general.utils.ts. The function properly handles address comparison with case normalization and special ETH address cases, making it suitable for both token and general address comparisons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of the new function name
rg "areTokenAddressesEqual" --type ts --type tsx
Length of output: 76
Script:
#!/bin/bash
# Search for both old and new function names in all files
rg "areTokenAddressesEqual|areAddressesEqual" -t ts -t typescript -t jsx -t tsx
# Also search for function definitions using AST
ast-grep --pattern 'function areTokenAddressesEqual($_) { $$$ }'
ast-grep --pattern 'function areAddressesEqual($_) { $$$ }'
ast-grep --pattern 'const areTokenAddressesEqual = $_'
ast-grep --pattern 'const areAddressesEqual = $_'
Length of output: 1203
src/components/Claim/Link/Initial.view.tsx (1)
302-302: LGTM: Consistent function usage in cross-chain detection
The function rename is correctly applied in the cross-chain detection logic.
src/utils/general.utils.ts (3)
Line range hint 371-379: Well-implemented address comparison function
The implementation is robust:
- Handles special cases for ETH addresses
- Uses
ethers.utils.getAddressfor case-insensitive comparison - Maintains consistent behavior with the previous implementation
382-382: LGTM: Consistent usage in zero address check
The function is correctly used in the zero address check utility.
981-982: LGTM: Proper integration in token symbol lookup
The function is appropriately used in the token symbol lookup logic.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json(4 hunks)
🔇 Additional comments (3)
package.json (3)
81-82: Testing setup looks complete and well-configured
The testing setup includes all necessary dependencies:
- Jest and its environment
- Testing Library
- TypeScript support via ts-jest
The configuration in the jest section is also properly set up with the correct test environment and module mapping.
Also applies to: 84-84, 95-96, 105-105
Line range hint 5-7: Node.js engine requirements are well-defined
The engine requirements are specific and include the latest LTS versions:
"engines": {
"node": ">=21.1.0 <22.0.0 || ^18.18.0 || ^20.9.0"
}This ensures compatibility with both the current LTS versions (18.x and 20.x) and the latest stable version (21.x).
107-107: Verify TypeScript version compatibility
The TypeScript version has been updated to ^5.6.3. Ensure this version is compatible with all your type definitions and Next.js version.
✅ Verification successful
TypeScript version update appears compatible
The TypeScript version update to ^5.6.3 appears safe based on:
- Next.js version (^14.2.18) is compatible with TypeScript 5.x
- No experimental TypeScript features are used in tsconfig.json
- Standard TypeScript configuration is used with common Next.js settings
- Codebase shows typical Next.js TypeScript patterns without advanced type features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential TypeScript compatibility issues
# Look for any TypeScript-related errors or warnings in package-lock.json
if [ -f package-lock.json ]; then
echo "Checking package-lock.json for TypeScript compatibility warnings..."
cat package-lock.json | jq -r '.packages | .[] | select(.name != null) | select(.name | contains("@types")) | {name: .name, version: .version}'
fi
# Check Next.js TypeScript template version
echo "Checking Next.js TypeScript template version..."
if [ -f tsconfig.json ]; then
cat tsconfig.json
fi
Length of output: 1180
Script:
#!/bin/bash
# Check Next.js version and other TypeScript-related dependencies
cat package.json | jq -r '.dependencies["next"], .dependencies["@types/node"], .dependencies["@types/react"], .dependencies["@types/react-dom"]'
# Look for any TypeScript errors in the codebase
fd -e ts -e tsx --exec head -n 1 {} \;
# Check for any TypeScript experimental features usage
rg -l "experimental" tsconfig.json
Length of output: 11969
Hugo0
force-pushed
the
feat/send-with-peanut-wallet
branch
from
eec3bb9 to
b3fee6b
Compare
| </div> | ||
| )} | ||
|
|
||
| <div className="flex w-full flex-row items-center justify-between px-2 text-h8 text-gray-1"> |
There was a problem hiding this comment.
This was duplicated after merging
| tokenDecimals = | ||
| peanutTokenDetails | ||
| .find((detail) => detail.chainId.toString() == chainId) | ||
| ?.tokens.find((token) => areTokenAddressesEqual(token.address, tokenAddress))?.decimals ?? 18 |
There was a problem hiding this comment.
.find((detail) => detail.chainId.toString() == chainId)
Nit Q: shouldn't this be triple equality?
| userBalances.some( | ||
| (balance) => areTokenAddressesEqual(balance.address, tokenAddress) && balance.chainId == chainId | ||
| ) | ||
| userBalances.some((balance) => areAddressesEqual(balance.address, tokenAddress) && balance.chainId == chainId) |
| return wallet?.walletProviderType === interfaces.WalletProviderType.PEANUT | ||
| } |
There was a problem hiding this comment.
nit: function name seems quite short - I'd prefer overly long but descriptive names over too short generic ones that might cause confusion down the line
There was a problem hiding this comment.
I rely a lot in typing a function isPeanut that receives a Wallet object is self describing to me
| @@ -1,16 +1,14 @@ | |||
| 'use client' | |||
There was a problem hiding this comment.
thought: we expect this context to be available everywhere right? I find the organization of src > context > walletContext a bit confusing. Especially when we have components > Global too. It's a bit hard to navigate so many folders, I think we might want to dedicate some time to restructuring this
There was a problem hiding this comment.
thought 2: I think it was nice to have a high level overview comment explaining the goal of this file / WalletProvider. Sad to see it gone
There was a problem hiding this comment.
Hmmmm I think the distintion between components and context is useful (even if in the end they are all components). I know that a context will have a state available everywhere and components will be reusable UI pieces. (I do agree that we have many folders and sometimes is hard to navigate, if you ask me to write a path to a component I am working on I wont be able to do it)
| if (selectedWallet) { | ||
| updateSelectedWalletWithConnectionStatus(selectedWallet) | ||
| if (!selectedAddress && wallets.length > 0) { | ||
| const initialWallet = wallets.find(isPeanut) ?? wallets[0] |
There was a problem hiding this comment.
Q: its smart to use peanut wallet as the default wallet, i like it. Question, do we respect users preferences? Ie should we store their last used wallet and let them use that instead on the next session?
There was a problem hiding this comment.
Hmmm I think that's a good Idea
| // TODO: if active, default active -> PW if logged in, otherwise | ||
| // if another wallet is connected to the provider, make that active, otherwise | ||
| // no wallet active (have to review all props to ensure they are null) | ||
| }, [wagmiAddress, wallets, user, addAccount]) |
There was a problem hiding this comment.
Q: doesn't having addAccount in dep array make this circular?
There was a problem hiding this comment.
addAccount should not change, is an util function, it should not be part of this depArray
| }) | ||
|
|
||
| const { data: wallets = [] } = useQuery<interfaces.IWallet[]>({ | ||
| queryKey: ['wallets', user?.accounts, wagmiAddress], |
There was a problem hiding this comment.
nit: queryKey should containt all dependencies of isWalletConnected
I added a dev dependency to detect this automatically
There was a problem hiding this comment.
nvm the linter doesn't seeem to work
2 participants
Main changes
Other changes
QA info
Right now it should be possible to generate a request link, fulfill it and then generate a claim link with that balance.
IMPORTANT: right now the token must be manually selected, in a (near)future PR we will hide the token selector. Select USDC in arbitrum as token.
Summary by CodeRabbit
Release Notes
New Features
Improvements
These updates aim to improve user experience and streamline interactions within the application.