Removed fee UI info and fixed native swap with fee#370
Conversation
WalkthroughThis change removes all fee estimation, calculation, and display logic from the ExchangeAction component and its tests. It refactors transaction preparation to use the user's portfolio instead of a cached native balance. Supporting utility functions for extracting balances from the portfolio are added, and the getStepTransactions function is updated to handle balances and fees based on the user's portfolio. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExchangeAction
participant useOffer
participant BlockchainUtils
User->>ExchangeAction: Click "Exchange"
ExchangeAction->>useOffer: getStepTransactions(token, route, account, portfolio)
useOffer->>BlockchainUtils: getTokenBalanceFromPortfolio / getNativeBalanceFromPortfolio
BlockchainUtils-->>useOffer: Return balances
useOffer-->>ExchangeAction: Return prepared transactions or error
ExchangeAction-->>User: Show result or error message
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Deploying x with
|
| Latest commit: |
4760f9f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://90df8ae8.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3384-fee-capure-the.x-e62.pages.dev |
![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
🧹 Nitpick comments (2)
src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx (1)
241-241: Remove debug statement before merging.The
screen.debug()call should be removed as it's typically used only during test development.- screen.debug();src/apps/the-exchange/hooks/useOffer.tsx (1)
333-334: Consider using optional chaining for cleaner code.The static analysis tool correctly identifies an opportunity to use optional chaining here.
- if (nativeFeeRoute && nativeFeeRoute.toAmount) { + if (nativeFeeRoute?.toAmount) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx(4 hunks)src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx(1 hunks)src/apps/the-exchange/hooks/useOffer.tsx(8 hunks)src/apps/the-exchange/utils/blockchain.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
src/apps/the-exchange/utils/blockchain.ts (1)
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx (1)
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
src/apps/the-exchange/hooks/useOffer.tsx (2)
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Learnt from: RanaBug
PR: pillarwallet/x#334
File: src/apps/leaderboard/utils/index.tsx:91-94
Timestamp: 2025-06-17T09:20:44.533Z
Learning: In src/apps/leaderboard/utils/index.tsx, the getLastWeekMigrationData function intentionally uses currentWeek (not lastWeek) for the completedSwapWeek lookup. This is correct business logic - when retrieving last week's migration data, the function should check swap completion against the current week while using lastWeek for points and USD calculations.
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
🧬 Code Graph Analysis (2)
src/apps/the-exchange/utils/blockchain.ts (2)
src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/apps/the-exchange/utils/wrappedTokens.ts (1)
isNativeToken(30-31)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
src/services/tokensData.ts (1)
convertPortfolioAPIResponseToToken(97-121)
🪛 Biome (1.9.4)
src/apps/the-exchange/hooks/useOffer.tsx
[error] 333-333: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (8)
src/apps/the-exchange/utils/blockchain.ts (3)
103-115: LGTM!The function correctly retrieves token balance from the portfolio by matching contract address and chain ID. The case-insensitive comparison and proper string conversion are implemented correctly.
118-120: LGTM!The utility correctly converts amounts to wei using viem's parseUnits with proper type handling.
123-135: LGTM!The function correctly identifies and retrieves native token balance using the isNativeToken utility.
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (2)
101-110: LGTM!The portfolio conversion is implemented correctly, using the appropriate utility function and handling the undefined case properly.
159-161: Good improvement to error handling.Moving
setIsAddingToBatch(false)to the finally block ensures the loading state is always cleared, even if an error occurs.src/apps/the-exchange/hooks/useOffer.tsx (3)
248-268: LGTM!The balance sufficiency checks correctly calculate required amounts for both native and ERC20 tokens, with clear error messages for insufficient balances.
393-416: Good improvement to approval logic.The updated logic correctly skips approval checks for native tokens, which never require allowance. This prevents unnecessary blockchain calls and improves efficiency.
497-498: Good improvement to error handling.Re-throwing the error allows the UI to handle it properly, which aligns with the error handling in the ExchangeAction component.
| if (!userSelectedNative) { | ||
| const tokenBalance = | ||
| getTokenBalanceFromPortfolio( | ||
| userPortfolio, | ||
| tokenToSwap.contract, | ||
| fromTokenChainId | ||
| ) || '0'; | ||
| userTokenBalance = BigInt(tokenBalance); | ||
| } |
There was a problem hiding this comment.
Fix incorrect token balance conversion.
The token balance is being converted directly to BigInt without considering the token's decimals. This will cause incorrect balance comparisons for tokens with decimals.
- userTokenBalance = BigInt(tokenBalance);
+ userTokenBalance = toWei(tokenBalance, tokenToSwap.decimals);📝 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 (!userSelectedNative) { | |
| const tokenBalance = | |
| getTokenBalanceFromPortfolio( | |
| userPortfolio, | |
| tokenToSwap.contract, | |
| fromTokenChainId | |
| ) || '0'; | |
| userTokenBalance = BigInt(tokenBalance); | |
| } | |
| if (!userSelectedNative) { | |
| const tokenBalance = | |
| getTokenBalanceFromPortfolio( | |
| userPortfolio, | |
| tokenToSwap.contract, | |
| fromTokenChainId | |
| ) || '0'; | |
| userTokenBalance = toWei(tokenBalance, tokenToSwap.decimals); | |
| } |
🤖 Prompt for AI Agents
In src/apps/the-exchange/hooks/useOffer.tsx between lines 235 and 243, the token
balance string is converted directly to BigInt without adjusting for the token's
decimals, leading to incorrect balance values. To fix this, parse the token
balance as a decimal number and scale it appropriately by the token's decimals
before converting to BigInt, ensuring accurate balance representation for tokens
with decimals.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/apps/the-exchange/hooks/useOffer.tsx (1)
324-324: Apply optional chaining for safer property access.The static analysis tool correctly identified that optional chaining should be used here for safer property access.
- if (nativeFeeRoute && nativeFeeRoute.toAmount) { + if (nativeFeeRoute?.toAmount) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx(4 hunks)src/apps/the-exchange/hooks/useOffer.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx
🧰 Additional context used
🧠 Learnings (1)
src/apps/the-exchange/hooks/useOffer.tsx (2)
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Learnt from: RanaBug
PR: pillarwallet/x#334
File: src/apps/leaderboard/utils/index.tsx:91-94
Timestamp: 2025-06-17T09:20:44.533Z
Learning: In src/apps/leaderboard/utils/index.tsx, the getLastWeekMigrationData function intentionally uses currentWeek (not lastWeek) for the completedSwapWeek lookup. This is correct business logic - when retrieving last week's migration data, the function should check swap completion against the current week while using lastWeek for points and USD calculations.
🪛 Biome (1.9.4)
src/apps/the-exchange/hooks/useOffer.tsx
[error] 324-324: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (8)
src/apps/the-exchange/hooks/useOffer.tsx (8)
31-36: Import additions look good.The new utility functions imported from blockchain utils will support the enhanced balance and fee calculations implemented in this hook.
194-195: Function signature change improves precision.The updated signature now accepts the full user portfolio and original input amount, enabling more accurate fee calculations and balance validations compared to the previous cached native balance approach.
206-210: Proper decimal handling for amount conversion.The conversion now correctly uses
parseUnitswith the token's decimal places, addressing the previous issue where token balances were converted directly to BigInt without considering decimals.
215-230: Enhanced token type detection improves fee logic.The explicit categorization of token types (native, wrapped, stablecoin) provides better clarity for fee calculation logic and reduces ambiguity in the fee handling process.
267-353: Fee step logic is comprehensive and well-structured.The fee calculation logic properly handles different token types with appropriate fee payment methods. The native fee estimation for non-stable ERC20 tokens includes a reasonable 1% buffer for slippage.
373-377: Wrapping logic correctly uses route amounts.The wrapping step properly uses the route's fromAmount for the deposit transaction, maintaining consistency with the routing logic.
383-407: Approval logic correctly excludes native tokens.The refined approval logic properly distinguishes between native and ERC20 tokens, correctly skipping allowance checks for native tokens while maintaining proper approval flow for ERC20 tokens.
489-489: Error re-throwing enables proper UI handling.Re-throwing errors after logging allows the UI layer to handle errors appropriately, which aligns with the PR objective of removing fee display logic from the UI components.
| // --- BALANCE CHECKS --- | ||
| // For native: need enough for swap + fee | ||
| // For ERC20: need enough ERC20 for swap | ||
| let userNativeBalance = BigInt(0); | ||
| try { | ||
| // Get native balance from portfolio | ||
| const nativeBalance = | ||
| getNativeBalanceFromPortfolio(userPortfolio, fromTokenChainId) || '0'; | ||
| userNativeBalance = toWei(nativeBalance, 18); | ||
| } catch (e) { | ||
| throw new Error('Unable to fetch balances for swap.'); | ||
| } | ||
|
|
||
| // Calculate total required | ||
| let totalNativeRequired = BigInt(0); | ||
| if (userSelectedNative) { | ||
| // Native: swap amount + fee | ||
| // Use fromAmountBigInt for total required | ||
| totalNativeRequired = fromAmountBigInt; | ||
| if (isWrapRequired) { | ||
| totalNativeRequired += fromAmountBigInt - feeAmount; // wrapping step uses swap amount | ||
| } | ||
| if (userNativeBalance < totalNativeRequired) { | ||
| throw new Error( | ||
| 'Insufficient native token balance to cover swap and fee.' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Balance validation logic needs refinement.
The balance checking logic has an issue with ERC20 token validation. For non-native tokens, you're only checking native balance but not validating if the user has sufficient ERC20 tokens for the swap.
Add ERC20 token balance validation:
// Calculate total required
let totalNativeRequired = BigInt(0);
if (userSelectedNative) {
// Native: swap amount + fee
// Use fromAmountBigInt for total required
totalNativeRequired = fromAmountBigInt;
if (isWrapRequired) {
totalNativeRequired += fromAmountBigInt - feeAmount; // wrapping step uses swap amount
}
if (userNativeBalance < totalNativeRequired) {
throw new Error(
'Insufficient native token balance to cover swap and fee.'
);
}
+ } else {
+ // ERC20 tokens: check if user has enough ERC20 tokens for swap
+ const tokenBalance = getTokenBalanceFromPortfolio(userPortfolio, tokenToSwap.contract, fromTokenChainId) || '0';
+ const userTokenBalance = toWei(tokenBalance, tokenToSwap.decimals);
+ if (userTokenBalance < fromAmountBigInt) {
+ throw new Error('Insufficient token balance for swap.');
+ }
}📝 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.
| // --- BALANCE CHECKS --- | |
| // For native: need enough for swap + fee | |
| // For ERC20: need enough ERC20 for swap | |
| let userNativeBalance = BigInt(0); | |
| try { | |
| // Get native balance from portfolio | |
| const nativeBalance = | |
| getNativeBalanceFromPortfolio(userPortfolio, fromTokenChainId) || '0'; | |
| userNativeBalance = toWei(nativeBalance, 18); | |
| } catch (e) { | |
| throw new Error('Unable to fetch balances for swap.'); | |
| } | |
| // Calculate total required | |
| let totalNativeRequired = BigInt(0); | |
| if (userSelectedNative) { | |
| // Native: swap amount + fee | |
| // Use fromAmountBigInt for total required | |
| totalNativeRequired = fromAmountBigInt; | |
| if (isWrapRequired) { | |
| totalNativeRequired += fromAmountBigInt - feeAmount; // wrapping step uses swap amount | |
| } | |
| if (userNativeBalance < totalNativeRequired) { | |
| throw new Error( | |
| 'Insufficient native token balance to cover swap and fee.' | |
| ); | |
| } | |
| } | |
| // Calculate total required | |
| let totalNativeRequired = BigInt(0); | |
| if (userSelectedNative) { | |
| // Native: swap amount + fee | |
| // Use fromAmountBigInt for total required | |
| totalNativeRequired = fromAmountBigInt; | |
| if (isWrapRequired) { | |
| totalNativeRequired += fromAmountBigInt - feeAmount; // wrapping step uses swap amount | |
| } | |
| if (userNativeBalance < totalNativeRequired) { | |
| throw new Error( | |
| 'Insufficient native token balance to cover swap and fee.' | |
| ); | |
| } | |
| } else { | |
| // ERC20 tokens: check if user has enough ERC20 tokens for swap | |
| const tokenBalance = getTokenBalanceFromPortfolio( | |
| userPortfolio, | |
| tokenToSwap.contract, | |
| fromTokenChainId | |
| ) || '0'; | |
| const userTokenBalance = toWei(tokenBalance, tokenToSwap.decimals); | |
| if (userTokenBalance < fromAmountBigInt) { | |
| throw new Error('Insufficient token balance for swap.'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/apps/the-exchange/hooks/useOffer.tsx around lines 232 to 259, the current
balance check only validates native token balances but does not verify if the
user has enough ERC20 tokens for the swap. To fix this, add logic to fetch the
user's ERC20 token balance from their portfolio and compare it against the
required swap amount. If the ERC20 balance is insufficient, throw an error
indicating the user lacks enough ERC20 tokens for the swap.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
New Features