feat/PRO-3384/fee-capure-the-exchange#369
Conversation
WalkthroughFee estimation and deduction are integrated into the swap flow, with 1% of the input token amount deducted and routed to a configured fee receiver. The UI now displays fee details and warnings, and error handling is improved. Supporting utilities for fee detection, stablecoin checks, and transaction decoding are added, alongside comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExchangeAction (UI)
participant useOffer (Hook)
participant LiFiSDK
participant Blockchain
User->>ExchangeAction (UI): Initiate swap
ExchangeAction (UI)->>useOffer (Hook): fetchFeeInfo()
useOffer (Hook)->>LiFiSDK: getStepTransactions (with 1% fee deduction)
LiFiSDK->>useOffer (Hook): Returns step transactions (includes fee step)
useOffer (Hook)->>ExchangeAction (UI): Return fee info
ExchangeAction (UI)->>User: Display fee and warning (if any)
User->>ExchangeAction (UI): Confirm swap
ExchangeAction (UI)->>useOffer (Hook): getStepTransactions (on swap)
useOffer (Hook)->>Blockchain: Execute fee payment & swap steps
Blockchain-->>ExchangeAction (UI): Transaction result
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: |
07c48f3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://61962636.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: 7
🧹 Nitpick comments (4)
src/utils/blockchain.ts (1)
312-347: Add documentation for STABLECOIN_ADDRESSESPlease add JSDoc comments to document the purpose and structure of this constant for better maintainability.
+/** + * Mapping of chain IDs to sets of stablecoin contract addresses. + * Used to identify stablecoin tokens for fee payment logic. + * Addresses are stored in lowercase for case-insensitive comparison. + */ const STABLECOIN_ADDRESSES: Record<number, Set<string>> = {src/apps/the-exchange/utils/blockchain.ts (1)
44-56: Extract ERC20 transfer method signature as a constantThe hard-coded method signature should be defined as a constant for better maintainability.
+// ERC20 transfer(address,uint256) method signature +const ERC20_TRANSFER_SIGNATURE = '0xa9059cbb'; + // Helper: Detect if a tx is an ERC20 (stablecoin) fee step export const isERC20FeeTx = ( tx: StepTransaction, swapToken: Token ): boolean => { return ( typeof tx.to === 'string' && typeof swapToken.contract === 'string' && tx.to.toLowerCase() === swapToken.contract.toLowerCase() && tx.value === BigInt(0) && typeof tx.data === 'string' && - tx.data.startsWith('0xa9059cbb') + tx.data.startsWith(ERC20_TRANSFER_SIGNATURE) ); };src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
183-195: Extract native balance fetching to reduce code duplicationThe native balance extraction logic is duplicated between
fetchFeeInfoandonClickToExchange.+const getNativeBalanceForChain = ( + walletPortfolio: PortfolioData | undefined, + chainId: number +) => { + return getNativeBalanceFromPortfolio( + walletPortfolio + ? convertPortfolioAPIResponseToToken(walletPortfolio) + : undefined, + chainId + ); +}; // In onClickToExchange: - const nativeBalance = getNativeBalanceFromPortfolio( - walletPortfolio - ? convertPortfolioAPIResponseToToken(walletPortfolio) - : undefined, - bestOffer.offer.fromChainId - ); + const nativeBalance = getNativeBalanceForChain( + walletPortfolio, + bestOffer.offer.fromChainId + );src/apps/the-exchange/hooks/useOffer.tsx (1)
53-58: Extract native token addresses as constantsThe hardcoded native token addresses should be defined as constants for better maintainability.
+const NATIVE_TOKEN_ADDRESSES = [ + '0x0000000000000000000000000000000000000000', + '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' +] as const; + export const getNativeBalanceFromPortfolio = ( walletPortfolio: Token[] | undefined, chainId: number ): string | undefined => { if (!walletPortfolio) return undefined; // Find the native token for the chain (by contract address) const nativeToken = walletPortfolio.find( (token) => chainNameToChainIdTokensData(token.blockchain) === chainId && - (token.contract === '0x0000000000000000000000000000000000000000' || - token.contract === '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee') + NATIVE_TOKEN_ADDRESSES.includes(token.contract.toLowerCase()) ); return nativeToken ? String(nativeToken.balance) : undefined; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/the-exchange/components/ExchangeAction/test/__snapshots__/ExchangeAction.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx(8 hunks)src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx(3 hunks)src/apps/the-exchange/hooks/useOffer.tsx(4 hunks)src/apps/the-exchange/utils/blockchain.ts(2 hunks)src/utils/blockchain.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
src/utils/blockchain.ts (1)
Learnt from: RanaBug
PR: pillarwallet/x#315
File: src/apps/the-exchange/utils/wrappedTokens.ts:6-20
Timestamp: 2025-05-23T14:44:33.911Z
Learning: XDAI (Gnosis Chain) is intentionally excluded from the WRAPPED_NATIVE_TOKEN_ADDRESSES mapping in the exchange app's wrappedTokens utility.
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/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.
src/apps/the-exchange/hooks/useOffer.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/apps/the-exchange/utils/types.tsx (1)
StepTransaction(33-42)src/services/tokensData.ts (1)
Token(19-29)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (7)
src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/apps/the-exchange/hooks/useReducerHooks.tsx (1)
useAppSelector(6-6)src/types/api.ts (1)
PortfolioData(731-740)src/apps/the-exchange/hooks/useOffer.tsx (1)
getNativeBalanceFromPortfolio(47-60)src/services/tokensData.ts (1)
convertPortfolioAPIResponseToToken(97-121)src/apps/the-exchange/utils/blockchain.ts (4)
isNativeFeeTx(32-41)isERC20FeeTx(44-56)getFeeAmount(59-82)getFeeSymbol(85-94)src/apps/the-exchange/utils/converters.ts (1)
formatTokenAmount(6-11)
🪛 Biome (1.9.4)
src/apps/the-exchange/hooks/useOffer.tsx
[error] 264-264: 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 (2)
src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx (1)
222-274: Well-structured tests for fee functionalityThe new tests properly cover fee display and error scenarios with appropriate mocking and assertions.
src/apps/the-exchange/hooks/useOffer.tsx (1)
226-231: Verify fee receiver address is configuredThe code should validate that the fee receiver address is properly configured before using it.
const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; + if (!feeReceiver || !isValidEthereumAddress(feeReceiver)) { + throw new Error('Invalid or missing fee receiver address configuration'); + } const feeAmount = BigInt(route.fromAmount) / BigInt(100); // 1%
| ); | ||
|
|
||
| useEffect(() => { | ||
| const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; |
There was a problem hiding this comment.
Could you let me know what this value should be please 🙏
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Just a few minor questions but looks good so far!
| amount: '0', | ||
| symbol: swapToken.symbol, | ||
| recipient: String(feeReceiver), | ||
| warning: 'Fee transaction not found. Please check your swap setup.', |
There was a problem hiding this comment.
Can i just check - who sees this message? If it's the user - then we need to be a bit more helpful than "check your swap setup" 🙏
There was a problem hiding this comment.
Oh yes you are right, this is a message to the user, I've changed it to something less techy and more useful
| setFeeInfo(null); | ||
| transactionDebugLog('Fee estimation error:', e); | ||
| setErrorMessage( | ||
| 'Unable to prepare the swap. Please check your wallet and try again.' |
There was a problem hiding this comment.
| 'Unable to prepare the swap. Please check your wallet and try again.' | |
| 'Unable to prepare the swap. Please check that you have enough funds in your wallet and try again' |
There was a problem hiding this comment.
I've changed this to something slightly different, because we already have a check before for enough funds
| (token) => | ||
| chainNameToChainIdTokensData(token.blockchain) === chainId && | ||
| (token.contract === '0x0000000000000000000000000000000000000000' || | ||
| token.contract === '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee') |
There was a problem hiding this comment.
Just wondering does this take into account Polygon POL token which i think is a different address?
There was a problem hiding this comment.
Good catch! I've added it and made the logic slighlty different with a isNative helper instead
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3384/fee-capure-the-exchange)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/apps/the-exchange/hooks/useOffer.tsx (3)
130-130: Make fee percentage configurable.The 1% fee is hardcoded. Consider making it configurable via environment variable for flexibility.
+const feePercentage = Number(import.meta.env.VITE_SWAP_FEE_PERCENTAGE || '1') / 100; -const fromAmountFeeDeducted = Number(fromAmount) * 0.99; +const fromAmountFeeDeducted = Number(fromAmount) * (1 - feePercentage);
279-279: Use optional chaining for better code safety.-if (nativeFeeRoute && nativeFeeRoute.toAmount) { +if (nativeFeeRoute?.toAmount) {
294-318: Extract error messages as constants and make buffer configurable.+const ERROR_MESSAGES = { + INSUFFICIENT_NATIVE_BALANCE: 'Insufficient native token balance to pay the fee. Please ensure you have enough native token to cover the fee.', + FEE_ESTIMATION_FAILED: 'Failed to estimate native fee for ERC20. No route found.', + GENERIC_FEE_ERROR: 'Failed to estimate native fee for ERC20.' +} as const; + +const FEE_BUFFER_PERCENTAGE = Number(import.meta.env.VITE_FEE_BUFFER_PERCENTAGE || '10');
🧹 Nitpick comments (2)
src/apps/the-exchange/hooks/useOffer.tsx (2)
66-109: Consider adding request caching and timeout handling.The
getNativeFeeForERC20function makes external API calls for fee estimation. Consider implementing request caching and timeout handling to improve performance and reliability.const getNativeFeeForERC20 = async ({ tokenAddress, chainId, feeAmount, slippage = 0.03, }: { tokenAddress: string; chainId: number; feeAmount: string; slippage?: number; }) => { try { + // Add timeout to prevent hanging requests + const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error('Request timeout')), 10000) + ); + const feeRouteRequest: RoutesRequest = { fromChainId: chainId, toChainId: chainId, fromTokenAddress: tokenAddress, toTokenAddress: zeroAddress, fromAmount: feeAmount, options: { slippage, bridges: { allow: ['relay'], }, exchanges: { allow: ['openocean', 'kyberswap'] }, }, }; - const result = await getRoutes(feeRouteRequest); + const result = await Promise.race([ + getRoutes(feeRouteRequest), + timeoutPromise + ]);
224-320: Consider performance optimization for fee calculation.The fee logic, especially for ERC20 tokens, involves multiple external API calls which could impact user experience. Consider implementing parallel processing or caching strategies.
For better performance, consider:
- Caching fee estimation results for similar token amounts
- Pre-calculating common fee scenarios
- Implementing fallback fee estimation methods
- Using batch requests where possible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx(8 hunks)src/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx(5 hunks)src/apps/the-exchange/hooks/useOffer.tsx(4 hunks)src/apps/the-exchange/utils/blockchain.ts(2 hunks)src/apps/the-exchange/utils/wrappedTokens.ts(1 hunks)src/utils/blockchain.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/blockchain.ts
- src/apps/the-exchange/utils/blockchain.ts
- src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx
🧰 Additional context used
🧠 Learnings (3)
src/apps/the-exchange/utils/wrappedTokens.ts (2)
Learnt from: RanaBug
PR: pillarwallet/x#315
File: src/apps/the-exchange/utils/wrappedTokens.ts:6-20
Timestamp: 2025-05-23T14:44:33.911Z
Learning: XDAI (Gnosis Chain) is intentionally excluded from the WRAPPED_NATIVE_TOKEN_ADDRESSES mapping in the exchange app's wrappedTokens utility.
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 (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/components/ExchangeAction/test/ExchangeAction.test.tsx (2)
src/store.ts (1)
store(65-77)src/apps/the-exchange/reducer/theExchangeSlice.ts (2)
setBestOffer(85-87)setSwapToken(73-75)
src/apps/the-exchange/hooks/useOffer.tsx (5)
src/services/tokensData.ts (2)
Token(19-29)chainNameToChainIdTokensData(234-255)src/apps/the-exchange/utils/wrappedTokens.ts (2)
isNativeToken(30-31)isWrappedToken(23-28)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/utils/blockchain.ts (1)
isStableCoin(350-355)src/apps/deposit/utils/blockchain.tsx (1)
getNativeBalance(154-181)
🪛 Biome (1.9.4)
src/apps/the-exchange/hooks/useOffer.tsx
[error] 279-279: 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 (9)
src/apps/the-exchange/utils/wrappedTokens.ts (1)
4-4: Polygon MATIC Native Address ConfirmedThe address
0x0000000000000000000000000000000000001010is already used insrc/services/tokensData.tsto identify MATIC and matches the common sentinel for Polygon’s native token (cf. 1inch). No further changes are needed.• Added in
src/apps/the-exchange/utils/wrappedTokens.tstoNATIVE_TOKEN_ADDRESSES
• Already referenced insrc/services/tokensData.tsfor MATIC detectionsrc/apps/the-exchange/components/ExchangeAction/test/ExchangeAction.test.tsx (6)
91-91: LGTM: Well-defined fee receiver constant.The
FEE_RECEIVERconstant provides a clear, reusable address for testing fee functionality.
225-256: LGTM: Comprehensive native token fee test.The test properly mocks the
useOfferhook to simulate native token fee scenarios and verifies the UI displays the correct fee information.
258-317: LGTM: Thorough stablecoin fee test with proper ERC20 encoding.The test correctly uses
encodeFunctionDatato simulate ERC20 transfer calls and validates the fee display for stablecoin scenarios with proper decimal handling.
319-378: LGTM: Wrapped token fee test with accurate contract address.The test uses the correct WETH contract address and properly tests the wrapped token fee scenario.
380-431: LGTM: ERC20 native fallback test covers edge case.The test properly validates the fallback behavior for non-stable ERC20 tokens where the fee is paid in native tokens.
433-451: LGTM: Error handling test ensures robust UI behavior.The test properly validates error display when
getStepTransactionsfails, ensuring users receive appropriate feedback.src/apps/the-exchange/hooks/useOffer.tsx (2)
43-45: LGTM: Utility function for wei conversion.The
toWeifunction provides a clean wrapper aroundparseUnitsfor wei conversion.
47-60: LGTM: Helper function for native balance extraction.The
getNativeBalanceFromPortfoliofunction efficiently extracts native token balance from wallet portfolio using theisNativeTokenhelper.
| const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; | ||
| const feeAmount = BigInt(route.fromAmount) / BigInt(100); // 1% |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for fee receiver address.
The fee receiver address comes from environment variable without validation. Consider adding address validation to prevent potential issues.
const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER;
+if (!feeReceiver || !feeReceiver.match(/^0x[a-fA-F0-9]{40}$/)) {
+ throw new Error('Invalid or missing fee receiver address');
+}
const feeAmount = BigInt(route.fromAmount) / BigInt(100); // 1%📝 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.
| const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; | |
| const feeAmount = BigInt(route.fromAmount) / BigInt(100); // 1% | |
| const feeReceiver = import.meta.env.VITE_SWAP_FEE_RECEIVER; | |
| if (!feeReceiver || !feeReceiver.match(/^0x[a-fA-F0-9]{40}$/)) { | |
| throw new Error('Invalid or missing fee receiver address'); | |
| } | |
| const feeAmount = BigInt(route.fromAmount) / BigInt(100); // 1% |
🤖 Prompt for AI Agents
In src/apps/the-exchange/hooks/useOffer.tsx around lines 226 to 227, the
feeReceiver address is assigned directly from an environment variable without
validation. Add validation logic to check if feeReceiver is a valid address
format before using it. If invalid, handle the error appropriately, such as
throwing an error or using a fallback, to prevent potential issues from an
incorrect or malformed address.
| ? cachedNativeBalance | ||
| : await getNativeBalance(fromAccount, fromTokenChainId); | ||
|
|
||
| const userNativeBalance = toWei(userNativeBalanceStr, 18); |
There was a problem hiding this comment.
Handle potential error in native balance conversion.
The toWei function might fail if userNativeBalanceStr contains an error string (as returned by getNativeBalance on failure).
const userNativeBalance = toWei(userNativeBalanceStr, 18);
+
+// Check if balance fetch failed
+if (userNativeBalanceStr.includes('Error')) {
+ throw new Error('Failed to fetch native token balance');
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apps/the-exchange/hooks/useOffer.tsx at line 291, the call to toWei with
userNativeBalanceStr may throw an error if userNativeBalanceStr contains an
error string from a failed getNativeBalance call. To fix this, add a check
before calling toWei to ensure userNativeBalanceStr is a valid numeric string.
If it is an error string or invalid, handle the error gracefully by setting a
default value or returning early to avoid calling toWei with invalid input.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores