Conversation
## Walkthrough
The update introduces paymaster-based fee payment to the `SendModalTokensTabView` component, enabling gasless and ERC20 token fee options. It adds state management, UI controls for fee type and asset selection, backend integration for paymaster data and gas price fetching, approval calldata computation, and validation logic. Supporting error messages and a new gasless service module are also included.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx | Extended to support paymaster-based fee payment, including new state variables, UI controls for fee type/asset selection, paymaster logic, approval data calculation, Redux integration for wallet portfolio, and enhanced transaction batching logic. |
| src/translations/en.json | Added new error messages for gasless transaction issues and new labels for fee type and asset selection. |
| src/services/gasless.ts | New module providing types and functions to fetch paymaster data and gas prices, including filtering by chain and token, plus error handling. |
| .env.example | Added environment variables for RPC URL, feature toggle, and paymaster service URL. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant SendModalTokensTabView
participant ReduxStore
participant GaslessService
participant EtherspotBatches
User->>SendModalTokensTabView: Opens Send Modal
SendModalTokensTabView->>ReduxStore: Fetch wallet portfolio
SendModalTokensTabView->>GaslessService: getAllGaslessPaymasters(chainId, tokens)
GaslessService-->>SendModalTokensTabView: Returns paymaster list
SendModalTokensTabView->>User: Render fee type and fee asset selectors (if paymaster mode)
User->>SendModalTokensTabView: Selects fee type and fee asset
SendModalTokensTabView->>GaslessService: getGasPrice(chainId)
GaslessService-->>SendModalTokensTabView: Returns gas price
SendModalTokensTabView->>SendModalTokensTabView: Calculate approval calldata
User->>SendModalTokensTabView: Confirms send
SendModalTokensTabView->>EtherspotBatches: Submit transaction(s) with paymaster config and approval tx if neededPossibly related PRs
Suggested reviewers
Poem
|
Deploying x with
|
| Latest commit: |
d2bb0de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dc5e7e6e.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3185-gas-abstraction.x-e62.pages.dev |
| useWalletAddress, | ||
| } from '@etherspot/transaction-kit'; | ||
| import { BigNumber, ethers } from 'ethers'; | ||
| import { BigNumber, ethers, utils } from 'ethers'; |
There was a problem hiding this comment.
Can we replace ethers with viem? Otherwise we will end up with dependencies for both of these libraries in the PillarX binary.
There was a problem hiding this comment.
we can remove once we get rid of ethers from pillarX itself since we should be adding helper functions just like how we need in sdk to convert string/number to bigInt and then only we can use viem. The reason is that ethers BigNumber had a BigNumberish class which takes in both
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Outdated
Show resolved
Hide resolved
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/translations/en.json (2)
49-51: Consider phrasing the new error keys more generically
"insufficientBalanceForGasless"and"paymasterNotSupported"are tightly coupled to the current implementation details.
If other chains / paymaster modes are introduced later these keys might need to be reused (or replaced) with different wording.
A more generic wording such as"insufficientBalanceForPaymaster"and"paymasterBatchingNotSupported"would keep the i18n surface stable.
75-76: Key naming consistencyEvery other label key follows the pattern
select<X>. Using"selectFeeAsset"is consistent 👌, but"feeType"breaks that pattern ("selectFeeType"would be aligned with the rest). Renaming now saves a future breaking-change in the UI.src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
212-293: Missing cleanup & race-condition guard in paymaster discovery
fetch(...).then(...setState)may attempt to update state after the component unmounts or after another asset selection outraces the first request.
Add anAbortControllerand anisCancelledflag to prevent memory leaks and wrong option lists.
692-701: Unsafe parsing ofSelectOption.id
handleOnChangesplitsidon'-'assuming exactly four parts.
If a token address itself contains'-'(e.g. ENS name or future formatting) the parsing will break.Consider serialising the payload as JSON in
id(or place the extra data intovalue) rather than relying on string splitting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(13 hunks)src/translations/en.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (7)
src/components/Form/AssetSelect/index.tsx (1)
TokenAssetSelectOption(36-41)src/types/api.ts (1)
PortfolioData(723-732)src/apps/the-exchange/reducer/theExchangeSlice.ts (1)
setWalletPortfolio(107-112)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(13-35)src/services/tokensData.ts (2)
Token(11-21)chainNameToChainIdTokensData(237-258)src/utils/blockchain.ts (1)
isPolygonAssetNative(55-58)src/components/Form/Select/index.tsx (2)
SelectOption(11-17)Select(65-136)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
| const amountLeft = +getAmountLeft( | ||
| selectedAsset, | ||
| amount, | ||
| selectedAssetBalance | ||
| ); | ||
| if (!feeMin) return; | ||
| if (amountLeft < +feeMin) { | ||
| setErrorMessage(t`error.insufficientBalanceForGasless`); | ||
| setIsSending(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Balance check uses the wrong token
amountLeft is calculated from the asset being sent, but the fee is paid with selectedFeeAsset.
If a user sends token A and pays fees with token B the check will be meaningless (and can block valid transfers).
-const amountLeft = +getAmountLeft(
- selectedAsset,
- amount,
- selectedAssetBalance
-);
-if (!feeMin) return;
-if (amountLeft < +feeMin) {
+if (!selectedFeeAsset || !feeMin) return;
+const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[
+ accountAddress as string
+]?.find((b) =>
+ addressesEqual(b.token, selectedFeeAsset.token)
+ || (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null)
+)?.balance ?? 0;
+
+const feeTokenBalanceFormatted = +ethers.utils.formatUnits(
+ feeTokenBalance,
+ selectedFeeAsset.decimals,
+);
+
+if (feeTokenBalanceFormatted < +feeMin) {
setErrorMessage(t`error.insufficientBalanceForGasless`);
setIsSending(false);
return;
}📝 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 amountLeft = +getAmountLeft( | |
| selectedAsset, | |
| amount, | |
| selectedAssetBalance | |
| ); | |
| if (!feeMin) return; | |
| if (amountLeft < +feeMin) { | |
| setErrorMessage(t`error.insufficientBalanceForGasless`); | |
| setIsSending(false); | |
| return; | |
| } | |
| } | |
| if (!selectedFeeAsset || !feeMin) return; | |
| const feeTokenBalance = accountBalances | |
| ?.[selectedAsset?.chainId ?? 0] | |
| ?.[accountAddress as string] | |
| ?.find( | |
| (b) => | |
| addressesEqual(b.token, selectedFeeAsset.token) || | |
| (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) | |
| ) | |
| ?.balance ?? 0; | |
| const feeTokenBalanceFormatted = +ethers.utils.formatUnits( | |
| feeTokenBalance, | |
| selectedFeeAsset.decimals, | |
| ); | |
| if (feeTokenBalanceFormatted < +feeMin) { | |
| setErrorMessage(t`error.insufficientBalanceForGasless`); | |
| setIsSending(false); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 476 to 487, the balance check incorrectly compares the amount left of the
sent asset against the fee minimum, but fees are paid with selectedFeeAsset.
Update the calculation to check the balance of selectedFeeAsset instead of
selectedAsset when comparing against feeMin, ensuring the fee balance validation
is done on the correct token to avoid blocking valid transfers.
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Outdated
Show resolved
Hide resolved
| const getGasPrice = async (chainId: number) => { | ||
| const gasRes = await fetch( | ||
| `https://rpc.etherspot.io/v2/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| method: 'skandha_getGasPrice', | ||
| }), | ||
| } | ||
| ); | ||
| gasRes.json().then((response) => { | ||
| if (response.result) { | ||
| const totalGasPrice = BigNumber.from(response.result.maxFeePerGas) | ||
| .add(response.result.maxPriorityFeePerGas) | ||
| .toString(); | ||
| setGasPrice(totalGasPrice); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getGasPrice lacks error handling and can leave unresolved promises
The function fires fetch and afterwards chains .json().then(...) without a surrounding try / catch.
A network- or JSON-parsing failure will raise an uncaught Promise rejection and break React’s error boundary.
-const gasRes = await fetch(
+let gasRes: Response;
+try {
+ gasRes = await fetch(
`https://rpc.etherspot.io/v2/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`,
{
...
}
);
- gasRes.json().then((response) => {
- if (response.result) {
- ...
- }
- });
+ const response = await gasRes.json();
+ if (response?.result) {
+ const totalGasPrice = BigNumber
+ .from(response.result.maxFeePerGas)
+ .add(response.result.maxPriorityFeePerGas)
+ .toString();
+ setGasPrice(totalGasPrice);
+ }
+} catch (e) {
+ console.error('Failed to fetch gas price', e);
+ // fallback – disable paymaster rather than crashing UI
+ setGasPrice(undefined);
+}📝 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 getGasPrice = async (chainId: number) => { | |
| const gasRes = await fetch( | |
| `https://rpc.etherspot.io/v2/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| method: 'skandha_getGasPrice', | |
| }), | |
| } | |
| ); | |
| gasRes.json().then((response) => { | |
| if (response.result) { | |
| const totalGasPrice = BigNumber.from(response.result.maxFeePerGas) | |
| .add(response.result.maxPriorityFeePerGas) | |
| .toString(); | |
| setGasPrice(totalGasPrice); | |
| } | |
| }); | |
| }; | |
| const getGasPrice = async (chainId: number) => { | |
| let gasRes: Response; | |
| try { | |
| gasRes = await fetch( | |
| `https://rpc.etherspot.io/v2/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| method: 'skandha_getGasPrice', | |
| }), | |
| } | |
| ); | |
| const response = await gasRes.json(); | |
| if (response?.result) { | |
| const totalGasPrice = BigNumber | |
| .from(response.result.maxFeePerGas) | |
| .add(response.result.maxPriorityFeePerGas) | |
| .toString(); | |
| setGasPrice(totalGasPrice); | |
| } | |
| } catch (e) { | |
| console.error('Failed to fetch gas price', e); | |
| // fallback – disable paymaster rather than crashing UI | |
| setGasPrice(undefined); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx between
lines 187 and 210, the getGasPrice function lacks error handling around the
fetch call and the subsequent JSON parsing, which can cause uncaught promise
rejections on network or parsing failures. To fix this, wrap the entire async
operation in a try/catch block, await the fetch and json parsing properly, and
handle any errors by catching exceptions and optionally logging or setting an
error state to prevent unhandled promise rejections.
github-actions
bot
temporarily deployed
to
Preview (PRO-3185-Gas_Abstraction)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (4)
16-16: Consider migrating from ethers to viem.The PR introduces both ethers and viem libraries. This creates a dual dependency situation, which can lead to increased bundle size and maintenance overhead.
As noted in a previous review, consider replacing ethers with viem to standardize on a single library throughout the codebase. This would reduce bundle size and simplify maintenance.
187-210: Fix error handling in the getGasPrice function.The
getGasPricefunction lacks proper error handling around the fetch call and JSON parsing, potentially causing uncaught promise rejections.As noted in a previous review, the function needs error handling to prevent uncaught Promise rejections. Wrap the fetch and JSON parsing in try/catch and set a fallback state when errors occur.
493-505: Fix incorrect balance check for paymaster fees.The current implementation compares the amount left of the sent asset against the fee minimum, but fees are paid with the
selectedFeeAsset.As previously noted, the balance check uses the wrong token.
amountLeftis calculated from the asset being sent, but the fee is paid withselectedFeeAsset. Update the calculation to check the balance ofselectedFeeAssetinstead.
781-794: Reorder approve transaction to precede the primary transaction.For ERC-20 paymaster mode, the approve transaction must come before the primary transaction, otherwise the paymaster validation will fail.
As previously noted, the
approvetransaction must precede the primary transaction. Reorder the transaction components so the approval comes first for the paymaster to work correctly.
🧹 Nitpick comments (5)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (5)
125-125: Consider moving this URL to a global constant.Hardcoded URLs should be defined as constants at a global level for easier maintenance and consistency.
Move this and other service URLs to a global constants file to make them easier to update across the application.
127-145: Refactor complex state management using custom hooks.The component has numerous state variables that could be better organized with custom hooks to improve code maintainability.
Consider creating a custom hook like
usePaymasterStateto encapsulate all these related state variables:// hooks/usePaymasterState.ts export function usePaymasterState() { const [isPaymaster, setIsPaymaster] = React.useState<boolean>(false); const [paymasterContext, setPaymasterContext] = React.useState<{ mode: string; token?: string; } | null>({ mode: 'sponsor' }); const [selectedPaymasterAddress, setSelectedPaymasterAddress] = React.useState<string>(''); const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ token: string; decimals: number; }>(); // ... other state variables return { isPaymaster, setIsPaymaster, paymasterContext, setPaymasterContext, // ... other variables and setters }; }
710-732: Improve handling of fee asset and fee type selection.The current handlers contain logic that should be extracted into helper functions for better organization and testability.
Refactor these handlers for clarity and separation of concerns:
const handleFeeAssetSelection = (value: SelectOption) => { const [tokenAddress, , paymasterAddress, decimals] = value.id.split('-'); setSelectedFeeAsset({ token: tokenAddress, decimals: Number(decimals) || 18, }); setSelectedPaymasterAddress(paymasterAddress); }; const handleFeeTypeSelection = (value: SelectOption) => { const isGaslessMode = value.title === 'Gasless'; if (isGaslessMode) { setPaymasterContext({ mode: 'commonerc20', token: selectedFeeAsset?.token, }); setIsPaymaster(true); } else { setPaymasterContext(null); setIsPaymaster(false); } };
323-365: Extract environment variable constants to a configuration file.The code contains multiple direct references to environment variables for gas-related constants.
Move these to a dedicated configuration file:
// config/gasConstants.ts export const GAS_CONSUMPTION = { NATIVE: { DEFAULT: process.env.REACT_APP_NATIVE_GAS_CONSUMPTION, ARBITRUM: process.env.REACT_APP_NATIVE_GAS_CONSUMPTION_ARBITRUM, }, TOKEN: { DEFAULT: process.env.REACT_APP_TOKEN_GAS_CONSUMPTION, ARBITRUM: process.env.REACT_APP_TOKEN_GAS_CONSUMPTION_ARBITRUM, }, NFT: { DEFAULT: process.env.REACT_APP_NFT_GAS_CONSUMPTION, ARBITRUM: process.env.REACT_APP_NFT_GAS_CONSUMPTION_ARBITRUM, }, };Then simplify the code using this configuration:
const chainType = selectedAsset.chainId === 42161 ? 'ARBITRUM' : 'DEFAULT'; const assetType = selectedAsset.asset.contract === ethers.constants.AddressZero ? 'NATIVE' : 'TOKEN'; gasCost = Number(GAS_CONSUMPTION[assetType][chainType]);
734-744: Simplify the effect for updating context when fee asset changes.This effect has a simple logic that can be improved for readability.
Simplify this effect:
- useEffect(() => { - if (!selectedFeeAsset) return; - if (isPaymaster && paymasterContext?.mode === 'commonerc20') { - setPaymasterContext({ - mode: 'commonerc20', - token: selectedFeeAsset.token, - }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [selectedFeeAsset]); + useEffect(() => { + if (selectedFeeAsset && isPaymaster && paymasterContext?.mode === 'commonerc20') { + setPaymasterContext(prev => ({ + ...prev, + token: selectedFeeAsset.token, + })); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [selectedFeeAsset]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
| `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistently handle API keys and endpoint URLs.
There are inconsistencies in how API endpoints and keys are constructed in different parts of the code.
Create utility functions for constructing API URLs and queries:
// utils/apiUtils.ts
export function getPaymasterApiUrl(endpoint: string, params?: Record<string, string>) {
const baseUrl = process.env.REACT_APP_PAYMASTER_URL;
const apiKey = process.env.REACT_APP_PAYMASTER_API_KEY;
const queryParams = new URLSearchParams({ apiKey });
if (params) {
Object.entries(params).forEach(([key, value]) => {
queryParams.append(key, value);
});
}
return `${baseUrl}/${endpoint}?${queryParams.toString()}`;
}Then use this consistently throughout the code:
const url = getPaymasterApiUrl('getAllCommonERC20PaymasterAddress');Also applies to: 217-218, 220-220
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 141-142 and also lines 217-218 and 220, the construction of API endpoints
and inclusion of API keys is inconsistent. To fix this, create a utility
function (e.g., getPaymasterApiUrl) that takes an endpoint and optional query
parameters, reads the base URL and API key from environment variables, and
returns a properly formatted URL with all query parameters including the API
key. Replace all direct string constructions of these URLs in the specified
lines with calls to this utility function to ensure consistency and
maintainability.
| fetch( | ||
| `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, | ||
| { | ||
| method: 'POST', | ||
| body: JSON.stringify({}), | ||
| } | ||
| ).then((res) => { | ||
| res.json().then((data) => { | ||
| if (data.message) { | ||
| let paymasterObject = JSON.parse(data.message); | ||
| paymasterObject = paymasterObject.filter( | ||
| (item: { epVersion: string; chainId: number; gasToken: string }) => | ||
| item.epVersion === 'EPV_07' && | ||
| item.chainId === selectedAsset?.chainId && | ||
| tokens.find( | ||
| (token: Token) => | ||
| token.contract === item.gasToken.toLowerCase() || | ||
| (isPolygonAssetNative(item.gasToken, item.chainId) && | ||
| token.contract === ethers.constants.AddressZero) | ||
| ) | ||
| ); | ||
| const feeOptions = paymasterObject.map( | ||
| (item: { | ||
| gasToken: string; | ||
| chainId: number; | ||
| epVersion: string; | ||
| paymasterAddress: string; | ||
| // eslint-disable-next-line consistent-return, array-callback-return | ||
| }) => { | ||
| const tokenData = tokens.find( | ||
| (token: Token) => token.contract === item.gasToken.toLowerCase() | ||
| ); | ||
| if (tokenData) | ||
| return { | ||
| id: `${item.gasToken}-${item.chainId}-${item.paymasterAddress}-${tokenData.decimals}`, | ||
| type: 'token', | ||
| title: tokenData.name, | ||
| imageSrc: tokenData.logo, | ||
| chainId: chainNameToChainIdTokensData(tokenData.blockchain), | ||
| value: tokenData.balance, | ||
| price: tokenData.price, | ||
| asset: { | ||
| ...tokenData, | ||
| contract: item.gasToken, | ||
| decimals: tokenData.decimals, | ||
| }, | ||
| }; | ||
| } | ||
| ); | ||
| setFeeAssetOptions(feeOptions); | ||
| if (feeOptions.length > 0) { | ||
| // get Skandha gas price | ||
| getGasPrice(selectedAsset.chainId); | ||
| setSelectedFeeAsset({ | ||
| token: feeOptions[0].asset.contract, | ||
| decimals: feeOptions[0].asset.decimals, | ||
| }); | ||
| setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]); | ||
| setPaymasterContext({ | ||
| mode: 'commonerc20', | ||
| token: feeOptions[0].asset.contract, | ||
| }); | ||
| setIsPaymaster(true); | ||
| } else { | ||
| setFeeType([]); | ||
| } | ||
| } else { | ||
| setPaymasterContext(null); | ||
| setIsPaymaster(false); | ||
| setFeeType([]); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor paymaster address fetching for better error handling and cleaner code.
This block has several issues:
- Lacks proper error handling for fetch
- Contains deeply nested code that's hard to follow
- Uses synchronous JSON.parse on potentially large data
Refactor with async/await, proper error handling, and extract filtering logic:
- fetch(
- `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`,
- {
- method: 'POST',
- body: JSON.stringify({}),
- }
- ).then((res) => {
- res.json().then((data) => {
- if (data.message) {
- let paymasterObject = JSON.parse(data.message);
- // ... more nested code ...
- } else {
- setPaymasterContext(null);
- setIsPaymaster(false);
- setFeeType([]);
- }
- });
- });
+ try {
+ const res = await fetch(
+ `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`,
+ {
+ method: 'POST',
+ body: JSON.stringify({}),
+ }
+ );
+
+ const data = await res.json();
+
+ if (!data.message) {
+ setPaymasterContext(null);
+ setIsPaymaster(false);
+ setFeeType([]);
+ return;
+ }
+
+ const paymasterObject = JSON.parse(data.message);
+ const filteredPaymasters = filterCompatiblePaymasters(paymasterObject, selectedAsset, tokens);
+ const feeOptions = mapPaymastersToFeeOptions(filteredPaymasters, tokens);
+
+ setFeeAssetOptions(feeOptions);
+
+ if (feeOptions.length > 0) {
+ setupPaymasterWithFirstOption(feeOptions, selectedAsset);
+ } else {
+ setFeeType([]);
+ }
+ } catch (error) {
+ console.error('Failed to fetch paymaster addresses:', error);
+ setPaymasterContext(null);
+ setIsPaymaster(false);
+ setFeeType([]);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx between
lines 219 and 291, refactor the fetch call to use async/await syntax instead of
nested then calls for better readability. Add try/catch blocks to handle fetch
and JSON parsing errors gracefully. Extract the filtering and mapping logic for
paymaster objects into separate helper functions to reduce nesting and improve
clarity. Replace synchronous JSON.parse with safer parsing inside try/catch to
handle malformed data without crashing. Ensure all error cases update state
appropriately to reflect failure or empty results.
| {isPaymaster && | ||
| selectedPaymasterAddress && | ||
| selectedFeeAsset && | ||
| approveData && ( | ||
| <EtherspotTransaction | ||
| to={selectedFeeAsset.token} | ||
| data={approveData} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure paymaster approval transaction is rendered correctly.
The approval transaction component has a potentially unnecessary check for approveData, which might not be initialized if there are errors in fetching gas prices or token prices.
Add appropriate error handling and user feedback:
{isPaymaster &&
selectedPaymasterAddress &&
selectedFeeAsset &&
- approveData && (
+ (
+ <>
+ {!approveData && (
+ <ErrorMessage>Error generating approval transaction. Gas price data may be unavailable.</ErrorMessage>
+ )}
+ {approveData && (
<EtherspotTransaction
to={selectedFeeAsset.token}
data={approveData}
/>
+ )}
+ </>
)}📝 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.
| {isPaymaster && | |
| selectedPaymasterAddress && | |
| selectedFeeAsset && | |
| approveData && ( | |
| <EtherspotTransaction | |
| to={selectedFeeAsset.token} | |
| data={approveData} | |
| /> | |
| )} | |
| {isPaymaster && | |
| selectedPaymasterAddress && | |
| selectedFeeAsset && ( | |
| <> | |
| {!approveData && ( | |
| <ErrorMessage> | |
| Error generating approval transaction. Gas price data may be unavailable. | |
| </ErrorMessage> | |
| )} | |
| {approveData && ( | |
| <EtherspotTransaction | |
| to={selectedFeeAsset.token} | |
| data={approveData} | |
| /> | |
| )} | |
| </> | |
| )} |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 1003 to 1011, the rendering of the paymaster approval transaction depends
on the presence of approveData, which may be undefined due to errors in fetching
gas or token prices. To fix this, add error handling to detect when approveData
is missing or invalid and provide user feedback such as an error message or a
loading indicator. This ensures the UI gracefully handles cases where
approveData is not available instead of silently skipping the transaction
rendering.
| const setApprovalData = async (gasCost: number) => { | ||
| if (selectedFeeAsset && gasPrice) { | ||
| const estimatedCost = Number( | ||
| utils.formatEther(BigNumber.from(gasCost).mul(gasPrice)) | ||
| ); | ||
| const costAsFiat = +estimatedCost * nativeAssetPrice; | ||
| const feeTokenPrice = await getPrices([selectedFeeAsset.token]); | ||
| let estimatedCostInToken; | ||
| if (feeTokenPrice) { | ||
| const feeTokenPriceInUSD = feeTokenPrice[0].usd; | ||
| estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed( | ||
| selectedFeeAsset.decimals | ||
| ); | ||
| setFeeMin(estimatedCostInToken); | ||
| setApproveData( | ||
| encodeFunctionData({ | ||
| abi: erc20Abi, | ||
| functionName: 'approve', | ||
| args: [ | ||
| selectedPaymasterAddress as Address, | ||
| parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), | ||
| ], | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance price fetching with error handling and confirmation.
The setApprovalData function lacks proper error handling and may calculate incorrect approval amounts if token price data is unavailable.
Add error handling and validation to the price fetching:
const setApprovalData = async (gasCost: number) => {
if (selectedFeeAsset && gasPrice) {
try {
const estimatedCost = Number(
utils.formatEther(BigNumber.from(gasCost).mul(gasPrice))
);
const costAsFiat = +estimatedCost * nativeAssetPrice;
const feeTokenPrice = await getPrices([selectedFeeAsset.token]);
+ if (!feeTokenPrice || !feeTokenPrice[0]?.usd) {
+ console.error('Failed to fetch token price');
+ return;
+ }
const feeTokenPriceInUSD = feeTokenPrice[0].usd;
const estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed(
selectedFeeAsset.decimals
);
setFeeMin(estimatedCostInToken);
setApproveData(
encodeFunctionData({
abi: erc20Abi,
functionName: 'approve',
args: [
selectedPaymasterAddress as Address,
parseUnits(estimatedCostInToken, selectedFeeAsset.decimals),
],
})
);
+ } catch (error) {
+ console.error('Failed to generate approval data:', error);
+ }
}
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx between
lines 295 and 321, the setApprovalData function fetches token prices without
error handling, risking incorrect approval amounts if price data is missing. To
fix this, add try-catch around the getPrices call to handle potential errors
gracefully, and validate that feeTokenPrice and feeTokenPrice[0].usd exist
before using them. If price data is unavailable, avoid setting approval data or
set a safe default to prevent incorrect calculations.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (5)
16-16: Consider replacing ethers with viem to standardize dependencies.The code is currently using both ethers.js and viem libraries, which could lead to dependency bloat in the final bundle.
187-210:⚠️ Potential issuegetGasPrice lacks error handling for fetch and JSON parsing.
The function doesn't handle network failures or JSON parsing errors, which could lead to unhandled promise rejections and break React's error boundary.
-const getGasPrice = async (chainId: number) => { - const gasRes = await fetch( - `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - jsonrpc: '2.0', - id: 1, - method: 'skandha_getGasPrice', - }), - } - ); - gasRes.json().then((response) => { - if (response.result) { - const totalGasPrice = BigNumber.from(response.result.maxFeePerGas) - .add(response.result.maxPriorityFeePerGas) - .toString(); - setGasPrice(totalGasPrice); - } - }); +const getGasPrice = async (chainId: number) => { + try { + const gasRes = await fetch( + `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, + { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'skandha_getGasPrice', + }), + } + ); + + const response = await gasRes.json(); + + if (response?.result) { + const totalGasPrice = BigNumber.from(response.result.maxFeePerGas) + .add(response.result.maxPriorityFeePerGas) + .toString(); + setGasPrice(totalGasPrice); + } else { + console.error('Invalid gas price response', response); + setGasPrice(undefined); + } + } catch (error) { + console.error('Failed to fetch gas price', error); + setGasPrice(undefined); + } };
140-142: 🛠️ Refactor suggestionHard-coded API key in queryString state initialization.
The API key is directly included in the component's state initialization which isn't ideal for configuration management and security best practices.
- const [queryString, setQueryString] = React.useState<string>( - `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}` - ); + const [queryString, setQueryString] = React.useState<string>(''); + + // Initialize queryString with API key from environment variables + useEffect(() => { + setQueryString(`?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`); + }, []);
493-505:⚠️ Potential issueBalance check uses the wrong token for fee validation.
The code is checking the balance of the selected asset being sent rather than the selected fee asset, which could block valid transfers.
if (isPaymaster && paymasterContext?.mode === 'commonerc20') { - const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance - ); - if (!feeMin) return; - if (amountLeft < +feeMin) { + if (!selectedFeeAsset || !feeMin) return; + const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string + ]?.find((b) => + addressesEqual(b.token, selectedFeeAsset.token) + || (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) + )?.balance ?? 0; + + const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, + ); + + if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; } }
295-321: 🛠️ Refactor suggestionImprove error handling in setApprovalData function.
The function lacks proper error handling when fetching token prices and generating approval data.
const setApprovalData = async (gasCost: number) => { if (selectedFeeAsset && gasPrice) { + try { const estimatedCost = Number( utils.formatEther(BigNumber.from(gasCost).mul(gasPrice)) ); const costAsFiat = +estimatedCost * nativeAssetPrice; const feeTokenPrice = await getPrices([selectedFeeAsset.token]); - let estimatedCostInToken; - if (feeTokenPrice) { - const feeTokenPriceInUSD = feeTokenPrice[0].usd; + + if (!feeTokenPrice || !feeTokenPrice[0]?.usd) { + console.error('Failed to fetch token price or price is zero'); + return; + } + + const feeTokenPriceInUSD = feeTokenPrice[0].usd; + if (feeTokenPriceInUSD <= 0) { + console.error('Invalid token price', feeTokenPriceInUSD); + return; + } + estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed( selectedFeeAsset.decimals ); setFeeMin(estimatedCostInToken); setApproveData( encodeFunctionData({ abi: erc20Abi, functionName: 'approve', args: [ selectedPaymasterAddress as Address, parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), ], }) ); - } + } catch (error) { + console.error('Failed to generate approval data:', error); + setFeeMin(undefined); + setApproveData(''); + } } };
🧹 Nitpick comments (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
991-999: Approval transaction may not render if approveData is undefined.The conditional check for
approveDatacould prevent the approval transaction from rendering if there are errors in fetching gas prices or token prices.{isPaymaster && selectedPaymasterAddress && - selectedFeeAsset && - approveData && ( + selectedFeeAsset && ( + <> + {!approveData && ( + <div style={{ color: 'red', fontSize: '12px', marginBottom: '10px' }}> + Error generating approval data. Gas price data may be unavailable. + </div> + )} + {approveData && ( <EtherspotTransaction to={selectedFeeAsset.token} data={approveData} /> + )} + </> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (7)
src/components/Form/AssetSelect/index.tsx (1)
TokenAssetSelectOption(36-41)src/types/api.ts (1)
PortfolioData(723-732)src/apps/the-exchange/reducer/theExchangeSlice.ts (1)
setWalletPortfolio(107-112)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(13-35)src/services/tokensData.ts (2)
Token(11-21)chainNameToChainIdTokensData(237-258)src/utils/blockchain.ts (1)
isPolygonAssetNative(55-58)src/components/Form/Select/index.tsx (2)
SelectOption(11-17)Select(65-136)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
781-793: Approve transaction is correctly placed before the primary transaction.The implementation correctly places the approval transaction before the primary transaction when using a paymaster, which is essential for ERC-20 paymaster mode to function properly.
141-142:Details
❓ Verification inconclusive
API key exposed in frontend code.
The API key is directly included in the frontend code, which poses a security risk as it can be exposed to clients.
Verify if this API key requires additional protection:
If the API key provides significant access, consider moving token authentication to a backend service or implementing token rotation.
🏁 Script executed:
#!/bin/bash # Check if the API key is client-side safe or requires backend protection # Look for any rate limiting or access control in the codebase grep -r "REACT_APP_PAYMASTER_API_KEY" --include="*.js" --include="*.jsx" --include="*.ts" --include="*.tsx" . # Check environment files for API key configuration cat .env* 2>/dev/null | grep PAYMASTERLength of output: 624
Confirm API Key Exposure in Client Build
It looks like
REACT_APP_PAYMASTER_API_KEYis injected directly into the client bundle:• src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
–?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}(line 141)
–?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true(line 145)
–${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}(line 148)This will expose the API key to end users. If this key grants any privileged access, please:
- Validate its scope and privileges.
- Proxy these requests through a secure backend or serverless function to keep the key secret.
- Consider token rotation or scoped tokens with minimal permissions.
Please verify whether this key is safe to expose; otherwise migrate these calls to a backend layer.
| `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true` | ||
| ); | ||
| fetch( | ||
| `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent API URL construction.
The code has inconsistencies in how API endpoints and keys are constructed, which makes maintenance more difficult.
fetch(
- `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`,
+ `${paymasterUrl}/getAllCommonERC20PaymasterAddress${queryString}`,
{
method: 'POST',
body: JSON.stringify({}),
}
)Consider creating a utility function to handle API URL construction consistently:
// utils/apiUtils.ts
export function getPaymasterApiUrl(endpoint: string, params?: Record<string, string>) {
const baseUrl = process.env.REACT_APP_PAYMASTER_URL;
const apiKey = process.env.REACT_APP_PAYMASTER_API_KEY;
const queryParams = new URLSearchParams({ apiKey });
if (params) {
Object.entries(params).forEach(([key, value]) => {
queryParams.append(key, value);
});
}
return `${baseUrl}/${endpoint}?${queryParams.toString()}`;
}🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 217 to 220, the API URLs are constructed inconsistently with repeated
manual concatenation of base URLs and query parameters. To fix this, create a
utility function that accepts an endpoint and optional parameters, appends the
API key and other query parameters consistently, and returns the full URL
string. Replace all manual URL constructions in this file with calls to this
utility function to ensure uniformity and easier maintenance.
| `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true` | ||
| ); | ||
| fetch( | ||
| `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, | ||
| { | ||
| method: 'POST', | ||
| body: JSON.stringify({}), | ||
| } | ||
| ).then((res) => { | ||
| res.json().then((data) => { | ||
| if (data.message) { | ||
| let paymasterObject = JSON.parse(data.message); | ||
| paymasterObject = paymasterObject.filter( | ||
| (item: { epVersion: string; chainId: number; gasToken: string }) => | ||
| item.epVersion === 'EPV_07' && | ||
| item.chainId === selectedAsset?.chainId && | ||
| tokens.find( | ||
| (token: Token) => | ||
| token.contract === item.gasToken.toLowerCase() || | ||
| (isPolygonAssetNative(item.gasToken, item.chainId) && | ||
| token.contract === ethers.constants.AddressZero) | ||
| ) | ||
| ); | ||
| const feeOptions = paymasterObject.map( | ||
| (item: { | ||
| gasToken: string; | ||
| chainId: number; | ||
| epVersion: string; | ||
| paymasterAddress: string; | ||
| // eslint-disable-next-line consistent-return, array-callback-return | ||
| }) => { | ||
| const tokenData = tokens.find( | ||
| (token: Token) => token.contract === item.gasToken.toLowerCase() | ||
| ); | ||
| if (tokenData) | ||
| return { | ||
| id: `${item.gasToken}-${item.chainId}-${item.paymasterAddress}-${tokenData.decimals}`, | ||
| type: 'token', | ||
| title: tokenData.name, | ||
| imageSrc: tokenData.logo, | ||
| chainId: chainNameToChainIdTokensData(tokenData.blockchain), | ||
| value: tokenData.balance, | ||
| price: tokenData.price, | ||
| asset: { | ||
| ...tokenData, | ||
| contract: item.gasToken, | ||
| decimals: tokenData.decimals, | ||
| }, | ||
| }; | ||
| } | ||
| ); | ||
| setFeeAssetOptions(feeOptions); | ||
| if (feeOptions.length > 0) { | ||
| // get Skandha gas price | ||
| getGasPrice(selectedAsset.chainId); | ||
| setSelectedFeeAsset({ | ||
| token: feeOptions[0].asset.contract, | ||
| decimals: feeOptions[0].asset.decimals, | ||
| }); | ||
| setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]); | ||
| setPaymasterContext({ | ||
| mode: 'commonerc20', | ||
| token: feeOptions[0].asset.contract, | ||
| }); | ||
| setIsPaymaster(true); | ||
| } else { | ||
| setFeeType([]); | ||
| } | ||
| } else { | ||
| setPaymasterContext(null); | ||
| setIsPaymaster(false); | ||
| setFeeType([]); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor paymaster address fetching for better error handling and cleaner code.
The paymaster address fetching contains deeply nested code that lacks proper error handling.
Consider refactoring this code to use async/await with proper error handling and extracting filtering logic to separate functions:
const fetchPaymasterAddresses = async () => {
try {
const res = await fetch(
`${paymasterUrl}/getAllCommonERC20PaymasterAddress${queryString}`,
{
method: 'POST',
body: JSON.stringify({}),
}
);
const data = await res.json();
if (!data.message) {
setPaymasterContext(null);
setIsPaymaster(false);
setFeeType([]);
return;
}
const paymasterObject = JSON.parse(data.message);
const filteredPaymasters = filterCompatiblePaymasters(paymasterObject, selectedAsset, tokens);
const feeOptions = mapPaymastersToFeeOptions(filteredPaymasters, tokens);
setFeeAssetOptions(feeOptions);
if (feeOptions.length > 0) {
setupPaymasterWithFirstOption(feeOptions);
} else {
setFeeType([]);
}
} catch (error) {
console.error('Failed to fetch paymaster addresses:', error);
setPaymasterContext(null);
setIsPaymaster(false);
setFeeType([]);
}
};🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx between
lines 217 and 291, refactor the paymaster address fetching code to use
async/await instead of nested then calls for better readability and error
handling. Extract the filtering logic into separate functions like
filterCompatiblePaymasters and mapPaymastersToFeeOptions to clean up the main
function. Add a try-catch block around the fetch and JSON parsing to handle
errors gracefully by logging the error and resetting relevant state variables
(setPaymasterContext, setIsPaymaster, setFeeType). Also, encapsulate the setup
of the first fee option into a helper function (setupPaymasterWithFirstOption)
to keep the main function concise.
| chainNameToChainIdTokensData, | ||
| Token, | ||
| } from '../../../services/tokensData'; | ||
| import { |
There was a problem hiding this comment.
You are using those hooks from the deposit app. There are several useAppDispatch() and useAppSelector() used in different redux slices, please select the right folder. According to the rest of your code, you should import those from the Exchange reducer, not from the Deposit Reducer.
|
|
||
| useEffect(() => { | ||
| if (walletPortfolioData && isWalletPortfolioDataSuccess) { | ||
| dispatch(setWalletPortfolio(walletPortfolioData?.result?.data)); |
There was a problem hiding this comment.
I think a separate slice should be created for the PillarX app, as interfering with the Exchange slice might causes issues. In my next PR I created this new PillarX slice with walletPortfolio data too. So once this is push I can make the swap between using walletPortfolio from the Exchange Slice and using it from the PillarX Slice.
| dispatch(setWalletPortfolio(walletPortfolioData?.result?.data)); | ||
| } | ||
| if (!isWalletPortfolioDataSuccess || walletPortfolioDataError) { | ||
| dispatch(setWalletPortfolio(undefined)); |
There was a problem hiding this comment.
Might need to add an error state here, so the user knows when there is an error with fetching the balances data from walletportfolio
There was a problem hiding this comment.
yeah sure will do
| }, | ||
| ]); | ||
|
|
||
| const getGasPrice = async (chainId: number) => { |
There was a problem hiding this comment.
I think those API calls function should be added in a separate folder and call in this component.
| if (isSendDisabled) return; | ||
| setErrorMessage(''); | ||
|
|
||
| if (isPaymaster) { |
There was a problem hiding this comment.
I am not sure to understand this logic of if isPaymaster is true, then show an error message?
There was a problem hiding this comment.
its only when you add a transaction to batch, it errors out just because that the paymaster can be used for the whole batch not just for a single transaction and on the transaction tab I can't see the whole batched transactions which are there. We would need to add the batched transactions to a global variable or have a mapping of all gaseless transactions and refactor the flow
github-actions
bot
temporarily deployed
to
Preview (PRO-3185-Gas_Abstraction)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
192-273: Nestedfetchchain lacks error handling & readabilityThis block still contains deeply nested
.then()chains without atry/catch, potentially leaking unhandled rejections and is hard to follow.
A previous review already suggested converting toasync/awaitwith proper error paths and helper functions.
275-301:setApprovalDataassumes price data is always presentIf
getPricesfails or returns an empty array,feeTokenPrice[0].usdwill throw.
Addtry/catchand guard checks as recommended earlier to avoid silent crashes.
473-485:⚠️ Potential issueBalance check uses the wrong token
amountLeftis derived from the asset being sent, but when paymaster mode =commonerc20the fee is deducted fromselectedFeeAsset.
The current check can block valid transfers or let insufficient balances through.This was pointed out previously and is still unresolved.
- const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance - ); - if (!feeMin) return; - if (amountLeft < +feeMin) { + if (!selectedFeeAsset || !feeMin) return; + + // balance of the token used to pay the fee + const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string + ]?.find((b) => + addressesEqual( + b.token ?? ethers.constants.AddressZero, + selectedFeeAsset.token, + ), + )?.balance ?? 0; + + const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, + ); + + if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)src/translations/en.json(2 hunks)src/utils/blockchain.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/translations/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
src/utils/blockchain.ts
Outdated
| export const getGasPrice = async (chainId: number) => { | ||
| let gasPrice = '0'; | ||
| const gasRes = await fetch( | ||
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| method: 'skandha_getGasPrice', | ||
| }), | ||
| } | ||
| ); | ||
| gasRes.json().then((response) => { | ||
| if (response.result) { | ||
| gasPrice = BigNumber.from(response.result.maxFeePerGas) | ||
| .add(response.result.maxPriorityFeePerGas) | ||
| .toString(); | ||
| } | ||
| }); | ||
| return gasPrice; | ||
| }; |
There was a problem hiding this comment.
getGasPrice always resolves with '0' & lacks error handling
gasRes.json() is executed asynchronously but the function returns before the promise settles – callers always receive '0'.
Additionally, network / parsing failures will raise unhandled promise rejections.
-export const getGasPrice = async (chainId: number) => {
- let gasPrice = '0';
- const gasRes = await fetch(
- `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`,
- {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- },
- body: JSON.stringify({
- jsonrpc: '2.0',
- id: 1,
- method: 'skandha_getGasPrice',
- }),
- }
- );
- gasRes.json().then((response) => {
- if (response.result) {
- gasPrice = BigNumber.from(response.result.maxFeePerGas)
- .add(response.result.maxPriorityFeePerGas)
- .toString();
- }
- });
- return gasPrice;
-};
+export const getGasPrice = async (chainId: number): Promise<string> => {
+ try {
+ const res = await fetch(
+ `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`,
+ {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ jsonrpc: '2.0',
+ id: 1,
+ method: 'skandha_getGasPrice',
+ }),
+ },
+ );
+
+ const json = await res.json();
+ if (!json?.result) return '0';
+
+ return BigNumber.from(json.result.maxFeePerGas)
+ .add(json.result.maxPriorityFeePerGas)
+ .toString();
+ } catch (e) {
+ console.error('Failed to fetch gas price', e);
+ return '0';
+ }
+};This ensures:
- The function only resolves once the value is known.
- Errors are caught and logged, preventing unhandled promise rejections.
📝 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.
| export const getGasPrice = async (chainId: number) => { | |
| let gasPrice = '0'; | |
| const gasRes = await fetch( | |
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| method: 'skandha_getGasPrice', | |
| }), | |
| } | |
| ); | |
| gasRes.json().then((response) => { | |
| if (response.result) { | |
| gasPrice = BigNumber.from(response.result.maxFeePerGas) | |
| .add(response.result.maxPriorityFeePerGas) | |
| .toString(); | |
| } | |
| }); | |
| return gasPrice; | |
| }; | |
| export const getGasPrice = async (chainId: number): Promise<string> => { | |
| try { | |
| const res = await fetch( | |
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| method: 'skandha_getGasPrice', | |
| }), | |
| }, | |
| ); | |
| const json = await res.json(); | |
| if (!json?.result) return '0'; | |
| return BigNumber.from(json.result.maxFeePerGas) | |
| .add(json.result.maxPriorityFeePerGas) | |
| .toString(); | |
| } catch (e) { | |
| console.error('Failed to fetch gas price', e); | |
| return '0'; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/utils/blockchain.ts around lines 291 to 315, the getGasPrice function
returns '0' immediately because gasRes.json() is handled asynchronously without
awaiting its result, causing the function to resolve before the gas price is
retrieved. To fix this, await the gasRes.json() call to get the response before
processing it, and wrap the fetch and parsing logic in a try-catch block to
handle and log any errors, ensuring the function only resolves after the gas
price is obtained or logs errors if they occur.
| if (value.title === 'Gasless') { | ||
| setPaymasterContext({ | ||
| mode: 'commonerc20', | ||
| token: selectedFeeAsset?.token, | ||
| }); | ||
| setIsPaymaster(true); | ||
| } else { | ||
| setPaymasterContext(null); | ||
| setIsPaymaster(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fee-type selector maps “Gasless” to ERC-20 mode
Choosing “Gasless” should configure the paymaster with mode: 'sponsor' and clear selectedFeeAsset.
At the moment it sets mode: 'commonerc20', which:
- Triggers unnecessary approval logic.
- Requests a fee-token balance from the user – the opposite of a gas-free experience.
- if (value.title === 'Gasless') {
- setPaymasterContext({
- mode: 'commonerc20',
- token: selectedFeeAsset?.token,
- });
- setIsPaymaster(true);
- } else {
+ if (value.title === 'Gasless') {
+ setPaymasterContext({ mode: 'sponsor' });
+ setSelectedFeeAsset(undefined);
+ setIsPaymaster(true);
+ } else {Test matrix:
- Gasless selected → no approve tx rendered,
paymasterContext.mode === 'sponsor'. - Native Token selected → paymaster disabled.
- ERC-20 fee asset selected →
mode === 'commonerc20'and approve tx rendered.
📝 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 (value.title === 'Gasless') { | |
| setPaymasterContext({ | |
| mode: 'commonerc20', | |
| token: selectedFeeAsset?.token, | |
| }); | |
| setIsPaymaster(true); | |
| } else { | |
| setPaymasterContext(null); | |
| setIsPaymaster(false); | |
| } | |
| }; | |
| if (value.title === 'Gasless') { | |
| // Use sponsor mode for true gasless and clear any selected fee token | |
| setPaymasterContext({ mode: 'sponsor' }); | |
| setSelectedFeeAsset(undefined); | |
| setIsPaymaster(true); | |
| } else { | |
| setPaymasterContext(null); | |
| setIsPaymaster(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 702 to 712, the "Gasless" fee-type incorrectly sets paymasterContext mode
to 'commonerc20' and does not clear selectedFeeAsset, causing unnecessary
approval logic and fee-token balance requests. Update the code so that when
"Gasless" is selected, set paymasterContext mode to 'sponsor' and clear
selectedFeeAsset to prevent approval transactions and fee-token balance checks.
For other cases, maintain the existing logic for native token and ERC-20
selections.
| `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true` | ||
| ); | ||
| fetch( | ||
| `${paymasterUrl}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, |
There was a problem hiding this comment.
The whole fetch flow, the feeOptions and setApprovalData could be imported from another file, otherwise it adds too much code and logic to this component
There was a problem hiding this comment.
feeOptions and setApprovalData cannot be imported since the data would change once you change the selectedAsset and it will anyways be fetched on each changes on selectedAsset. But yeah I can do fetching on api on another file
github-actions
bot
temporarily deployed
to
Preview (PRO-3185-Gas_Abstraction)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (8)
16-16: Consider replacing ethers with viem for consistency.You're importing both ethers and viem libraries, which adds unnecessary bundle size. Previous review comments also suggested replacing ethers with viem throughout the codebase.
What are the advantages of replacing ethers.js with viem in a React application?
962-972: Add error handling for missing approveData.The component renders the approval transaction only if
approveDatais present, but it doesn't handle the case where approval data could not be generated due to errors.{isPaymaster && selectedPaymasterAddress && selectedFeeAsset && - approveData && ( + ( + <> + {!approveData && ( + <div style={{ color: 'red', marginBottom: '10px' }}> + {t`error.failedToGenerateApprovalTransaction`} + </div> + )} + {approveData && ( <EtherspotTransaction to={selectedFeeAsset.token} data={approveData} /> + )} + </> )}
196-200: Simplify query string generation with utility function.The query string construction is duplicated and could be simplified using a utility function.
- setQueryString( - `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true` - ); + setQueryString( + getPaymasterQueryString({ + chainId: selectedAsset.chainId.toString(), + useVp: 'true' + }) + );
142-144: 🛠️ Refactor suggestionUse a utility function for API URL construction.
The API key is hardcoded in the state initialization, which isn't ideal for configuration management.
- const [queryString, setQueryString] = React.useState<string>( - `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}` - ); + import { getPaymasterQueryString } from '../../../utils/apiUtils'; + // elsewhere in the component + const [queryString, setQueryString] = React.useState<string>( + getPaymasterQueryString() + ); // In utils/apiUtils.ts: export function getPaymasterQueryString(additionalParams?: Record<string, string>): string { const base = `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`; if (!additionalParams) return base; return Object.entries(additionalParams).reduce( (query, [key, value]) => `${query}&${key}=${value}`, base ); }
267-293: 🛠️ Refactor suggestionAdd proper error handling to setApprovalData function.
The function doesn't handle edge cases where
feeTokenPriceis undefined or empty, or when price data is missing.const setApprovalData = async (gasCost: number) => { if (selectedFeeAsset && gasPrice) { + try { const estimatedCost = Number( utils.formatEther(BigNumber.from(gasCost).mul(gasPrice)) ); const costAsFiat = +estimatedCost * nativeAssetPrice; const feeTokenPrice = await getPrices([selectedFeeAsset.token]); - let estimatedCostInToken; - if (feeTokenPrice) { + + if (!feeTokenPrice || !feeTokenPrice[0]?.usd) { + console.error('Failed to fetch token price or price data is missing'); + return; + } + const feeTokenPriceInUSD = feeTokenPrice[0].usd; - estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed( + const estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed( selectedFeeAsset.decimals ); setFeeMin(estimatedCostInToken); setApproveData( encodeFunctionData({ abi: erc20Abi, functionName: 'approve', args: [ selectedPaymasterAddress as Address, parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), ], }) ); - } + } catch (error) { + console.error('Failed to generate approval data:', error); + } } };
694-704:⚠️ Potential issueFix incorrect paymaster mode for Gasless option.
When "Gasless" is selected, the code incorrectly sets the paymaster mode to 'commonerc20' instead of 'sponsor', which triggers unnecessary approval logic.
if (value.title === 'Gasless') { setPaymasterContext({ - mode: 'commonerc20', - token: selectedFeeAsset?.token, + mode: 'sponsor', }); + setSelectedFeeAsset(undefined); setIsPaymaster(true); } else { setPaymasterContext(null); setIsPaymaster(false); }
465-477:⚠️ Potential issueIncorrect balance check for fee payment.
The current implementation checks if the remaining amount of the asset being sent is sufficient to cover the fee, but the fee should be paid with the selected fee asset, not the asset being sent.
if (isPaymaster && paymasterContext?.mode === 'commonerc20') { - const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance - ); if (!feeMin) return; - if (amountLeft < +feeMin) { + if (!selectedFeeAsset) return; + + const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string + ]?.find((b) => + addressesEqual(b.token, selectedFeeAsset.token) + || (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) + )?.balance ?? 0; + + const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, + ); + + if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; } }
240-244: 🛠️ Refactor suggestionImprove error handling for gas price fetching.
The current implementation doesn't handle errors when fetching gas prices.
- getGasPrice(selectedAsset.chainId).then((res) => { - setGasPrice(res); - }); + try { + getGasPrice(selectedAsset.chainId) + .then((res) => { + setGasPrice(res); + }) + .catch((error) => { + console.error('Failed to fetch gas price:', error); + setErrorMessage(t`error.failedToFetchGasPrice`); + }); + } catch (error) { + console.error('Error initiating gas price fetch:', error); + setErrorMessage(t`error.failedToFetchGasPrice`); + }
🧹 Nitpick comments (6)
src/services/gasless.ts (3)
1-2: Unnecessary eslint disable directive.The
eslint-disable import/extensionsdirective doesn't seem necessary for this file since there are no direct file path imports with extensions.-/* eslint-disable import/extensions */
16-17: Parameter name uses inconsistent naming convention.The parameter
tokens_listuses snake_case while the rest of the codebase follows camelCase convention.export const getAllGaslessPaymasters = async ( chainId: number, - tokens_list: Token[] + tokensList: Token[] ): Promise<Paymasters[] | null> => {
46-51: Improve error handling in the catch block.The current catch block logs the error but doesn't provide any context about what operation failed.
} catch (err) { - console.error(err); + console.error('Failed to fetch or process paymaster data:', err); return null; }src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
127-148: Create a dedicated reducer for paymaster state management.The component introduces many state variables for paymaster functionality, making it harder to maintain and reason about. Consider using a reducer pattern to manage related state more effectively.
// Example paymaster reducer type PaymasterState = { isPaymaster: boolean; paymasterContext: { mode: string; token?: string } | null; selectedPaymasterAddress: string; selectedFeeAsset?: { token: string; decimals: number }; feeAssetOptions: TokenAssetSelectOption[]; queryString: string; approveData: string; gasPrice?: string; feeMin?: string; }; type PaymasterAction = | { type: 'SET_IS_PAYMASTER'; payload: boolean } | { type: 'SET_PAYMASTER_CONTEXT'; payload: { mode: string; token?: string } | null } | { type: 'SET_SELECTED_PAYMASTER_ADDRESS'; payload: string } | { type: 'SET_SELECTED_FEE_ASSET'; payload: { token: string; decimals: number } | undefined } | { type: 'SET_FEE_ASSET_OPTIONS'; payload: TokenAssetSelectOption[] } | { type: 'SET_QUERY_STRING'; payload: string } | { type: 'SET_APPROVE_DATA'; payload: string } | { type: 'SET_GAS_PRICE'; payload: string | undefined } | { type: 'SET_FEE_MIN'; payload: string | undefined }; const initialState: PaymasterState = { isPaymaster: false, paymasterContext: { mode: 'sponsor' }, selectedPaymasterAddress: '', selectedFeeAsset: undefined, feeAssetOptions: [], queryString: `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, approveData: '', gasPrice: undefined, feeMin: undefined, }; function paymasterReducer(state: PaymasterState, action: PaymasterAction): PaymasterState { // Implementation }
754-760: Add error handling for missing approveData in batched transactions.Similar to the previous comment, the component doesn't handle the case where approval data could not be generated for batched transactions.
{isPaymaster && selectedPaymasterAddress && selectedFeeAsset && ( + <> + {!approveData && ( + <div style={{ color: 'red', marginBottom: '10px' }}> + {t`error.failedToGenerateApprovalTransaction`} + </div> + )} + {approveData && ( <EtherspotTransaction to={selectedFeeAsset.token} data={approveData} /> + )} + </> )}
810-818: Add error handling for missing approveData in batched transactions.Similar to earlier comments, the component needs to handle cases where approval data could not be generated for batched transactions.
{isPaymaster && selectedPaymasterAddress && - approveData && selectedFeeAsset && ( + <> + {!approveData && ( + <div style={{ color: 'red', marginBottom: '10px' }}> + {t`error.failedToGenerateApprovalTransaction`} + </div> + )} + {approveData && ( <EtherspotTransaction to={selectedFeeAsset.token} data={approveData} /> + )} + </> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)src/services/gasless.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/gasless.ts (2)
src/services/tokensData.ts (1)
Token(11-21)src/utils/blockchain.ts (1)
isPolygonAssetNative(55-58)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
| let paymasterObject = JSON.parse(data.message); | ||
| paymasterObject = paymasterObject.filter( | ||
| (item: { epVersion: string; chainId: number; gasToken: string }) => | ||
| item.epVersion === 'EPV_07' && | ||
| item.chainId === chainId && | ||
| tokens_list.find( | ||
| (token: Token) => | ||
| token.contract === item.gasToken.toLowerCase() || | ||
| (isPolygonAssetNative(item.gasToken, item.chainId) && | ||
| token.contract === constants.AddressZero) | ||
| ) | ||
| ); | ||
| return paymasterObject; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for JSON parsing and make filtering more robust.
The current implementation lacks error handling for JSON parsing and assumes the filtered data has the expected structure.
- let paymasterObject = JSON.parse(data.message);
- paymasterObject = paymasterObject.filter(
- (item: { epVersion: string; chainId: number; gasToken: string }) =>
- item.epVersion === 'EPV_07' &&
- item.chainId === chainId &&
- tokens_list.find(
- (token: Token) =>
- token.contract === item.gasToken.toLowerCase() ||
- (isPolygonAssetNative(item.gasToken, item.chainId) &&
- token.contract === constants.AddressZero)
- )
- );
- return paymasterObject;
+ try {
+ let paymasterObject = JSON.parse(data.message);
+
+ if (!Array.isArray(paymasterObject)) {
+ console.error('Expected paymaster data to be an array');
+ return null;
+ }
+
+ paymasterObject = paymasterObject.filter(
+ (item: { epVersion?: string; chainId?: number; gasToken?: string }) =>
+ item.epVersion === 'EPV_07' &&
+ item.chainId === chainId &&
+ item.gasToken &&
+ tokensList.find(
+ (token: Token) =>
+ token.contract === item.gasToken!.toLowerCase() ||
+ (isPolygonAssetNative(item.gasToken!, item.chainId!) &&
+ token.contract === constants.AddressZero)
+ )
+ );
+ return paymasterObject;
+ } catch (parseErr) {
+ console.error('Failed to parse paymaster data:', parseErr);
+ return null;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/gasless.ts between lines 32 and 45, add a try-catch block around
the JSON.parse call to handle potential parsing errors gracefully. Inside the
try block, parse the JSON and perform the filtering as before. In the catch
block, log or handle the error appropriately and return an empty array or a safe
default. Additionally, before filtering, validate that the parsed data is an
array and that each item has the expected properties to avoid runtime errors
during filtering.
| const data = await res.json(); | ||
| // eslint-disable-next-line no-console | ||
| console.log(data.message); | ||
| if (data.message) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug console log and add response validation.
Remove debugging console.log and add validation to ensure data has the expected structure before parsing.
const data = await res.json();
- // eslint-disable-next-line no-console
- console.log(data.message);
- if (data.message) {
+ if (data && typeof data.message === 'string') {🤖 Prompt for AI Agents
In src/services/gasless.ts around lines 28 to 31, remove the debug console.log
statement to clean up the code. Add validation to check that the response data
has the expected structure, such as verifying that data is an object and
contains the message property, before accessing or parsing it to prevent runtime
errors.
| try { | ||
| // eslint-disable-next-line no-console | ||
| console.log(chainId, tokens_list); | ||
| const res = await fetch( | ||
| `${process.env.REACT_APP_PAYMASTER_URL}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, | ||
| { | ||
| method: 'POST', | ||
| body: JSON.stringify({}), | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console logs and improve error handling for the external API call.
Production code shouldn't contain console.log statements. Also, the fetch call lacks proper error handling and validation for the environment variables.
try {
- // eslint-disable-next-line no-console
- console.log(chainId, tokens_list);
+ if (!process.env.REACT_APP_PAYMASTER_URL || !process.env.REACT_APP_PAYMASTER_API_KEY) {
+ console.error('Paymaster URL or API key not configured');
+ return null;
+ }
const res = await fetch(
`${process.env.REACT_APP_PAYMASTER_URL}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`,
{
method: 'POST',
body: JSON.stringify({}),
}
);
+ if (!res.ok) {
+ console.error(`Failed to fetch paymaster data: ${res.status} ${res.statusText}`);
+ return null;
+ }📝 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.
| try { | |
| // eslint-disable-next-line no-console | |
| console.log(chainId, tokens_list); | |
| const res = await fetch( | |
| `${process.env.REACT_APP_PAYMASTER_URL}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| body: JSON.stringify({}), | |
| } | |
| ); | |
| try { | |
| if (!process.env.REACT_APP_PAYMASTER_URL || !process.env.REACT_APP_PAYMASTER_API_KEY) { | |
| console.error('Paymaster URL or API key not configured'); | |
| return null; | |
| } | |
| const res = await fetch( | |
| `${process.env.REACT_APP_PAYMASTER_URL}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| body: JSON.stringify({}), | |
| } | |
| ); | |
| if (!res.ok) { | |
| console.error(`Failed to fetch paymaster data: ${res.status} ${res.statusText}`); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In src/services/gasless.ts around lines 18 to 27, remove the console.log
statement used for debugging. Add checks to ensure that the environment
variables REACT_APP_PAYMASTER_URL and REACT_APP_PAYMASTER_API_KEY are defined
before making the fetch call. Implement proper error handling for the fetch
request by catching any errors and handling non-OK HTTP responses appropriately,
such as throwing an error or returning a fallback value.
| getAllGaslessPaymasters(selectedAsset.chainId, tokens).then( | ||
| (paymasterObject) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(paymasterObject); | ||
| if (paymasterObject) { | ||
| const feeOptions = paymasterObject | ||
| .map( | ||
| (item: { | ||
| gasToken: string; | ||
| chainId: number; | ||
| epVersion: string; | ||
| paymasterAddress: string; | ||
| // eslint-disable-next-line consistent-return, array-callback-return | ||
| }) => { | ||
| const tokenData = tokens.find( | ||
| (token: Token) => | ||
| token.contract === item.gasToken.toLowerCase() | ||
| ); | ||
| if (tokenData) | ||
| return { | ||
| id: `${item.gasToken}-${item.chainId}-${item.paymasterAddress}-${tokenData.decimals}`, | ||
| type: 'token', | ||
| title: tokenData.name, | ||
| imageSrc: tokenData.logo, | ||
| chainId: chainNameToChainIdTokensData(tokenData.blockchain), | ||
| value: tokenData.balance, | ||
| price: tokenData.price, | ||
| asset: { | ||
| ...tokenData, | ||
| contract: item.gasToken, | ||
| decimals: tokenData.decimals, | ||
| }, | ||
| } as TokenAssetSelectOption; | ||
| } | ||
| ) | ||
| .filter( | ||
| (value): value is TokenAssetSelectOption => value !== undefined | ||
| ); | ||
| if (feeOptions && feeOptions.length > 0 && feeOptions[0]) { | ||
| setFeeAssetOptions(feeOptions); | ||
| // get Skandha gas price | ||
| getGasPrice(selectedAsset.chainId).then((res) => { | ||
| setGasPrice(res); | ||
| }); | ||
| setSelectedFeeAsset({ | ||
| token: feeOptions[0].asset.contract, | ||
| decimals: feeOptions[0].asset.decimals, | ||
| }); | ||
| setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]); | ||
| setPaymasterContext({ | ||
| mode: 'commonerc20', | ||
| token: feeOptions[0].asset.contract, | ||
| }); | ||
| setIsPaymaster(true); | ||
| } else { | ||
| setFeeType([]); | ||
| } | ||
| } else { | ||
| setPaymasterContext(null); | ||
| setIsPaymaster(false); | ||
| setFeeType([]); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor paymaster setup with async/await and better error handling.
The paymaster setup code uses promises with .then(), making it harder to read and maintain. Refactor using async/await and add proper error handling.
You should consider refactoring this code to use async/await for better readability and error handling. Here's a simplified example of how part of this could be structured:
const setupPaymaster = async (chainId: number, tokens: Token[]) => {
try {
const paymasterObject = await getAllGaslessPaymasters(chainId, tokens);
if (!paymasterObject || paymasterObject.length === 0) {
setPaymasterContext(null);
setIsPaymaster(false);
setFeeType([]);
return;
}
const feeOptions = mapPaymastersToFeeOptions(paymasterObject, tokens);
if (feeOptions.length === 0) {
setFeeType([]);
return;
}
setFeeAssetOptions(feeOptions);
try {
const gasPrice = await getGasPrice(chainId);
setGasPrice(gasPrice);
} catch (error) {
console.error('Failed to fetch gas price:', error);
setErrorMessage(t`error.failedToFetchGasPrice`);
}
// Setup initial state with first option
setupInitialPaymasterState(feeOptions);
} catch (error) {
console.error('Failed to setup paymaster:', error);
setPaymasterContext(null);
setIsPaymaster(false);
setFeeType([]);
}
};🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 200 to 263, refactor the paymaster setup code from using .then() promise
chains to async/await syntax for improved readability and maintainability. Wrap
the entire logic in a try/catch block to handle errors gracefully. Await the
getAllGaslessPaymasters call, check for empty or null results to reset state
accordingly, then map the paymaster data to fee options. Await getGasPrice
inside a nested try/catch to handle gas price fetch errors separately. Finally,
set the initial paymaster state based on the first fee option. Remove
console.log statements and ensure all state updates happen within the async
function with proper error handling.
| useEffect(() => { | ||
| if (!walletPortfolio) return; | ||
| const tokens = convertPortfolioAPIResponseToToken(walletPortfolio); | ||
| if (!selectedAsset) return; | ||
| setQueryString( | ||
| `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true` | ||
| ); | ||
| getAllGaslessPaymasters(selectedAsset.chainId, tokens).then( | ||
| (paymasterObject) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(paymasterObject); | ||
| if (paymasterObject) { | ||
| const feeOptions = paymasterObject | ||
| .map( | ||
| (item: { | ||
| gasToken: string; | ||
| chainId: number; | ||
| epVersion: string; | ||
| paymasterAddress: string; | ||
| // eslint-disable-next-line consistent-return, array-callback-return | ||
| }) => { | ||
| const tokenData = tokens.find( | ||
| (token: Token) => | ||
| token.contract === item.gasToken.toLowerCase() | ||
| ); | ||
| if (tokenData) | ||
| return { | ||
| id: `${item.gasToken}-${item.chainId}-${item.paymasterAddress}-${tokenData.decimals}`, | ||
| type: 'token', | ||
| title: tokenData.name, | ||
| imageSrc: tokenData.logo, | ||
| chainId: chainNameToChainIdTokensData(tokenData.blockchain), | ||
| value: tokenData.balance, | ||
| price: tokenData.price, | ||
| asset: { | ||
| ...tokenData, | ||
| contract: item.gasToken, | ||
| decimals: tokenData.decimals, | ||
| }, | ||
| } as TokenAssetSelectOption; | ||
| } | ||
| ) | ||
| .filter( | ||
| (value): value is TokenAssetSelectOption => value !== undefined | ||
| ); | ||
| if (feeOptions && feeOptions.length > 0 && feeOptions[0]) { | ||
| setFeeAssetOptions(feeOptions); | ||
| // get Skandha gas price | ||
| getGasPrice(selectedAsset.chainId).then((res) => { | ||
| setGasPrice(res); | ||
| }); | ||
| setSelectedFeeAsset({ | ||
| token: feeOptions[0].asset.contract, | ||
| decimals: feeOptions[0].asset.decimals, | ||
| }); | ||
| setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]); | ||
| setPaymasterContext({ | ||
| mode: 'commonerc20', | ||
| token: feeOptions[0].asset.contract, | ||
| }); | ||
| setIsPaymaster(true); | ||
| } else { | ||
| setFeeType([]); | ||
| } | ||
| } else { | ||
| setPaymasterContext(null); | ||
| setIsPaymaster(false); | ||
| setFeeType([]); | ||
| } | ||
| } | ||
| ); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [selectedAsset, walletPortfolio]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract paymaster fetching logic into separate functions.
The useEffect for fetching and processing paymaster data is too complex and deeply nested. Extract this logic into separate, testable functions.
// Extracted functions should look something like:
const buildPaymasterQueryString = (chainId: number) =>
`?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${chainId}&useVp=true`;
const mapPaymasterToFeeOption = (
paymaster: { gasToken: string; chainId: number; paymasterAddress: string },
tokens: Token[]
): TokenAssetSelectOption | undefined => {
const tokenData = tokens.find(
(token: Token) => token.contract === paymaster.gasToken.toLowerCase()
);
if (!tokenData) return undefined;
return {
id: `${paymaster.gasToken}-${paymaster.chainId}-${paymaster.paymasterAddress}-${tokenData.decimals}`,
type: 'token',
title: tokenData.name,
imageSrc: tokenData.logo,
chainId: chainNameToChainIdTokensData(tokenData.blockchain),
value: tokenData.balance,
price: tokenData.price,
asset: {
...tokenData,
contract: paymaster.gasToken,
decimals: tokenData.decimals,
},
} as TokenAssetSelectOption;
};
const setupInitialPaymasterState = (feeOptions: TokenAssetSelectOption[], chainId: number) => {
if (feeOptions.length === 0) return;
// Setup initial state
getGasPrice(chainId).then(setGasPrice);
setSelectedFeeAsset({
token: feeOptions[0].asset.contract,
decimals: feeOptions[0].asset.decimals,
});
setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]);
setPaymasterContext({
mode: 'commonerc20',
token: feeOptions[0].asset.contract,
});
setIsPaymaster(true);
};🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 193 to 265, the useEffect hook contains complex and deeply nested logic
for fetching and processing paymaster data. To fix this, extract the query
string construction, mapping of paymaster objects to fee options, and the
initial paymaster state setup into separate reusable functions. Create a
function to build the paymaster query string from the chainId, another to map
each paymaster item to a TokenAssetSelectOption using the tokens array, and a
third to initialize state variables like gas price, selected fee asset,
paymaster address, context, and flags based on the fee options. Then replace the
inline logic in the useEffect with calls to these functions for improved
readability and testability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/services/gasless.ts (2)
18-31: 🛠️ Refactor suggestionRemove debug logs and add environment variable validation
The production code shouldn't contain console logs and should properly validate environment variables before using them.
try { - // eslint-disable-next-line no-console - console.log(chainId, tokens_list); + if (!process.env.REACT_APP_PAYMASTER_URL || !process.env.REACT_APP_PAYMASTER_API_KEY) { + console.error('Paymaster URL or API key not configured'); + return null; + } const res = await fetch( `${process.env.REACT_APP_PAYMASTER_URL}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, { method: 'POST', body: JSON.stringify({}), } ); + if (!res.ok) { + console.error(`Failed to fetch paymaster data: ${res.status} ${res.statusText}`); + return null; + } const data = await res.json(); - // eslint-disable-next-line no-console - console.log(data.message); - if (data.message) { + if (data && typeof data.message === 'string') {
32-45: 🛠️ Refactor suggestionImprove error handling for JSON parsing and filtering
The current implementation lacks error handling for JSON parsing and doesn't validate the paymaster data structure.
- let paymasterObject = JSON.parse(data.message); - paymasterObject = paymasterObject.filter( - (item: { epVersion: string; chainId: number; gasToken: string }) => - item.epVersion === 'EPV_07' && - item.chainId === chainId && - tokens_list.find( - (token: Token) => - token.contract === item.gasToken.toLowerCase() || - (isPolygonAssetNative(item.gasToken, item.chainId) && - token.contract === constants.AddressZero) - ) - ); - return paymasterObject; + try { + let paymasterObject = JSON.parse(data.message); + + if (!Array.isArray(paymasterObject)) { + console.error('Expected paymaster data to be an array'); + return null; + } + + paymasterObject = paymasterObject.filter( + (item: { epVersion?: string; chainId?: number; gasToken?: string }) => + item.epVersion === 'EPV_07' && + item.chainId === chainId && + item.gasToken && + tokens_list.find( + (token: Token) => + token.contract === item.gasToken!.toLowerCase() || + (isPolygonAssetNative(item.gasToken!, item.chainId!) && + token.contract === constants.AddressZero) + ) + ); + return paymasterObject; + } catch (parseErr) { + console.error('Failed to parse paymaster data:', parseErr); + return null; + }src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
467-479:⚠️ Potential issueBalance check uses the wrong token
The current implementation checks the balance of the asset being sent, but fees are paid with the
selectedFeeAsset. If a user sends token A and pays fees with token B, this check will be incorrect.-const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance -); -if (!feeMin) return; -if (amountLeft < +feeMin) { +if (!selectedFeeAsset || !feeMin) return; +const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string +]?.find((b) => + addressesEqual(b.token, selectedFeeAsset.token) + || (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) +)?.balance ?? 0; + +const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, +); + +if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; }
695-706:⚠️ Potential issueFee-type selector maps "Gasless" to ERC-20 mode
When "Gasless" is selected, it should configure the paymaster with
mode: 'sponsor'and clearselectedFeeAsset, but it incorrectly setsmode: 'commonerc20'.if (value.title === 'Gasless') { setPaymasterContext({ - mode: 'commonerc20', - token: selectedFeeAsset?.token, + mode: 'sponsor' }); + setSelectedFeeAsset(undefined); setIsPaymaster(true); } else { setPaymasterContext(null); setIsPaymaster(false); }
269-295: 🛠️ Refactor suggestionEnhance price fetching with error handling and validation
The
setApprovalDatafunction lacks proper error handling and may calculate incorrect approval amounts if token price data is unavailable.const setApprovalData = async (gasCost: number) => { if (selectedFeeAsset && gasPrice) { + try { const estimatedCost = Number( utils.formatEther(BigNumber.from(gasCost).mul(gasPrice)) ); const costAsFiat = +estimatedCost * nativeAssetPrice; const feeTokenPrice = await getPrices([selectedFeeAsset.token]); - let estimatedCostInToken; - if (feeTokenPrice) { + + if (!feeTokenPrice || !feeTokenPrice[0]?.usd) { + console.error('Failed to fetch token price'); + return; + } const feeTokenPriceInUSD = feeTokenPrice[0].usd; - estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed( + const estimatedCostInToken = (costAsFiat / feeTokenPriceInUSD).toFixed( selectedFeeAsset.decimals ); setFeeMin(estimatedCostInToken); setApproveData( encodeFunctionData({ abi: erc20Abi, functionName: 'approve', args: [ selectedPaymasterAddress as Address, parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), ], }) ); - } + } catch (error) { + console.error('Failed to generate approval data:', error); + } } };
🧹 Nitpick comments (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
195-267: Refactor paymaster setup with async/await for better readabilityThe paymaster setup code uses promises with
.then(), making it harder to read and maintain.Consider refactoring this code to use async/await:
useEffect(() => { const setupPaymaster = async () => { if (!walletPortfolio || !selectedAsset) return; const tokens = convertPortfolioAPIResponseToToken(walletPortfolio); setQueryString( `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}&chainId=${selectedAsset.chainId}&useVp=true` ); try { const paymasterObject = await getAllGaslessPaymasters(selectedAsset.chainId, tokens); if (!paymasterObject || paymasterObject.length === 0) { setPaymasterContext(null); setIsPaymaster(false); setFeeType([]); return; } const feeOptions = paymasterObject .map((item) => { // Map to fee options... const tokenData = tokens.find( (token) => token.contract === item.gasToken.toLowerCase() ); if (!tokenData) return undefined; return { id: `${item.gasToken}-${item.chainId}-${item.paymasterAddress}-${tokenData.decimals}`, type: 'token', title: tokenData.name, // ... other properties } as TokenAssetSelectOption; }) .filter((value): value is TokenAssetSelectOption => value !== undefined); if (feeOptions.length === 0) { setFeeType([]); return; } setFeeAssetOptions(feeOptions); try { const res = await getGasPrice(selectedAsset.chainId); setGasPrice(res); } catch (error) { console.error('Failed to fetch gas price:', error); } // Setup initial state with first option if (feeOptions[0]) { setSelectedFeeAsset({ token: feeOptions[0].asset.contract, decimals: feeOptions[0].asset.decimals, }); setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]); setPaymasterContext({ mode: 'commonerc20', token: feeOptions[0].asset.contract, }); setIsPaymaster(true); } } catch (error) { console.error('Failed to setup paymaster:', error); setPaymasterContext(null); setIsPaymaster(false); setFeeType([]); } }; setupPaymaster(); // eslint-disable-next-line react-hooks/exhaustive-deps }, [selectedAsset, walletPortfolio]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)src/services/gasless.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
757-762:approvetransaction must precede the primary transactionFor ERC-20 paymaster mode, the allowance must be set first. Currently, the approval transaction is correctly placed before the primary transaction. This is good implementation!
967-973: Ensure conditional rendering uses proper checksThe approval transaction depends on
approveDatabeing defined. This check correctly guards against rendering the transaction whenapproveDatais undefined, which is good practice.
src/services/gasless.ts
Outdated
| export const getGasPrice = async (chainId: number) => { | ||
| let gasPrice = '0'; | ||
| try { | ||
| const gasRes = await fetch( | ||
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| method: 'skandha_getGasPrice', | ||
| }), | ||
| } | ||
| ); | ||
| gasRes.json().then((response) => { | ||
| if (response.result) { | ||
| gasPrice = BigNumber.from(response.result.maxFeePerGas) | ||
| .add(response.result.maxPriorityFeePerGas) | ||
| .toString(); | ||
| } | ||
| }); | ||
| return gasPrice; | ||
| } catch (err) { | ||
| console.error(err); | ||
| return gasPrice; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Critical issue: getGasPrice doesn't wait for the async result
The function returns the default gas price ('0') immediately without waiting for the fetch promise to complete. The actual gas price calculation happens in the .then() callback, but it's never captured by the return value.
Replace the promise chain with async/await:
export const getGasPrice = async (chainId: number) => {
let gasPrice = '0';
try {
const gasRes = await fetch(
`${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`,
{
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({
jsonrpc: '2.0',
id: 1,
method: 'skandha_getGasPrice',
}),
}
);
- gasRes.json().then((response) => {
- if (response.result) {
- gasPrice = BigNumber.from(response.result.maxFeePerGas)
- .add(response.result.maxPriorityFeePerGas)
- .toString();
- }
- });
+ const response = await gasRes.json();
+ if (response?.result) {
+ gasPrice = BigNumber.from(response.result.maxFeePerGas)
+ .add(response.result.maxPriorityFeePerGas)
+ .toString();
+ }
return gasPrice;
} catch (err) {
console.error(err);
return gasPrice;
}
};📝 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.
| export const getGasPrice = async (chainId: number) => { | |
| let gasPrice = '0'; | |
| try { | |
| const gasRes = await fetch( | |
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| method: 'skandha_getGasPrice', | |
| }), | |
| } | |
| ); | |
| gasRes.json().then((response) => { | |
| if (response.result) { | |
| gasPrice = BigNumber.from(response.result.maxFeePerGas) | |
| .add(response.result.maxPriorityFeePerGas) | |
| .toString(); | |
| } | |
| }); | |
| return gasPrice; | |
| } catch (err) { | |
| console.error(err); | |
| return gasPrice; | |
| } | |
| }; | |
| export const getGasPrice = async (chainId: number) => { | |
| let gasPrice = '0'; | |
| try { | |
| const gasRes = await fetch( | |
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| jsonrpc: '2.0', | |
| id: 1, | |
| method: 'skandha_getGasPrice', | |
| }), | |
| } | |
| ); | |
| const response = await gasRes.json(); | |
| if (response?.result) { | |
| gasPrice = BigNumber.from(response.result.maxFeePerGas) | |
| .add(response.result.maxPriorityFeePerGas) | |
| .toString(); | |
| } | |
| return gasPrice; | |
| } catch (err) { | |
| console.error(err); | |
| return gasPrice; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/services/gasless.ts lines 53 to 82, the getGasPrice function returns the
default gas price '0' immediately because it does not await the asynchronous
JSON parsing and gas price calculation inside the .then() callback. To fix this,
replace the .then() promise chain with an await expression to wait for
gasRes.json() to resolve, then perform the gas price calculation synchronously
after that. Finally, return the calculated gas price after the await, ensuring
the function returns the correct value.
| const [queryString, setQueryString] = React.useState<string>( | ||
| `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded API key in queryString state
The API key is directly included in the component's state initialization, which isn't ideal for configuration management.
Extract this to a utility function:
// utils/apiHelpers.ts
export function getPaymasterQueryString(additionalParams?: Record<string, string>) {
const base = `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`;
if (!additionalParams) return base;
return Object.entries(additionalParams).reduce(
(query, [key, value]) => `${query}&${key}=${value}`,
base
);
}Then use this utility:
-const [queryString, setQueryString] = React.useState<string>(
- `?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`
-);
+const [queryString, setQueryString] = React.useState<string>(
+ getPaymasterQueryString()
+);🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 144 to 146, the API key is hard-coded directly in the queryString state
initialization. To fix this, create a utility function (e.g.,
getPaymasterQueryString) in a separate utils/apiHelpers.ts file that constructs
the query string including the API key and any additional parameters. Then,
replace the direct initialization of queryString with a call to this utility
function to improve configuration management and code reuse.
| useWalletAddress, | ||
| } from '@etherspot/transaction-kit'; | ||
| import { BigNumber, ethers } from 'ethers'; | ||
| import { BigNumber, ethers, utils } from 'ethers'; |
| if (selectedAsset.type === 'token') { | ||
| if (selectedAsset.asset.contract === ethers.constants.AddressZero) { | ||
| gasCost = Number( | ||
| process.env.REACT_APP_NATIVE_GAS_CONSUMPTION_ARBITRUM |
There was a problem hiding this comment.
Do the gas consumtion values need to be an environment variable as opposed to a const definition?
There was a problem hiding this comment.
Yes since if its const you would need code change to change every time rather you can just change the env and restart if you want to change the values in the middle
There was a problem hiding this comment.
Personally i don't see this as an issue (to store them in a const file or config object) as we can override this if we need to, but having these values in the control of infrastructure team means that we're dependent on them when we can just push an update ourselves, especally because it is not sensitive information. I'd recommend that we add these to a local file instead of pull them from the environment.
| process.env.REACT_APP_TOKEN_GAS_CONSUMPTION_ARBITRUM | ||
| ); // estimated gas consumption for token asset transfer for undeployed wallet + 15% markup | ||
| } | ||
| } else if (selectedAsset.type === 'nft') { | ||
| gasCost = Number(process.env.REACT_APP_NFT_GAS_CONSUMPTION_ARBITRUM); // estimated gas consumption for token asset transfer for undeployed wallet + 15% markup | ||
| } | ||
| } else { | ||
| // eslint-disable-next-line no-lonely-if | ||
| if (selectedAsset.type === 'token') { | ||
| if (selectedAsset.asset.contract === ethers.constants.AddressZero) { | ||
| gasCost = Number(process.env.REACT_APP_NATIVE_GAS_CONSUMPTION); // estimated gas consumption for native asset transfer for deployed wallet + 15% markup | ||
| } else { | ||
| gasCost = Number(process.env.REACT_APP_TOKEN_GAS_CONSUMPTION); // estimated gas consumption for token asset transfer for deployed wallet + 15% markup | ||
| } | ||
| } else if (selectedAsset.type === 'nft') { | ||
| gasCost = Number(process.env.REACT_APP_NFT_GAS_CONSUMPTION); // estimated gas consumption for token asset transfer for deployed wallet + 15% markup |
There was a problem hiding this comment.
The same applies for these process.env values
There was a problem hiding this comment.
The same ans from above works here too
src/services/gasless.ts
Outdated
| // eslint-disable-next-line no-console | ||
| console.log(chainId, tokens_list); | ||
| const res = await fetch( | ||
| `${process.env.REACT_APP_PAYMASTER_URL}/getAllCommonERC20PaymasterAddress?apiKey=${process.env.REACT_APP_PAYMASTER_API_KEY}`, |
There was a problem hiding this comment.
Is the paymaster proxy URL supposed to come from the Px API? If so is the API key needed here?
There was a problem hiding this comment.
ohh so this PR was raised before the Px API thats why apiKey is still there will remove it now as I created an API
src/services/gasless.ts
Outdated
| let gasPrice = '0'; | ||
| try { | ||
| const gasRes = await fetch( | ||
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, |
There was a problem hiding this comment.
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, | |
| `${process.env.REACT_APP_GAS_URL}/${chainId}?api-key=${process.env.REACT_APP_ETHERSPOT_BUNDLER_API_KEY}`, |
What is the GAS_URL supposed to be here?
There was a problem hiding this comment.
it the bundler url https://rpc.etherspot.io/v2
There was a problem hiding this comment.
Could we make this more specific please as GAS_URL os a bit generic
github-actions
bot
requested a deployment
to
Preview (PRO-3185-Gas_Abstraction)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
468-480:⚠️ Potential issueBalance validation still uses incorrect token.
This is the same issue flagged in previous reviews. The balance check compares
selectedAssetamount againstselectedFeeAssetfee minimum, but when using paymaster mode, users pay fees withselectedFeeAsset, notselectedAsset.-const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance -); -if (!feeMin) return; -if (amountLeft < +feeMin) { +if (!selectedFeeAsset || !feeMin) return; + +const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string +]?.find((b) => + addressesEqual(b.token, selectedFeeAsset.token) + || (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) +)?.balance ?? 0; + +const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, +); + +if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; }
717-722:⚠️ Potential issue"Gasless" fee type incorrectly configured.
The "Gasless" option should set paymaster mode to 'sponsor' and clear the selected fee asset, but it's currently setting mode to 'commonerc20' which triggers approval logic.
if (value.title === 'Gasless') { setPaymasterContext({ - mode: 'commonerc20', - token: selectedFeeAsset?.token, + mode: 'sponsor', }); + setSelectedFeeAsset(undefined); setIsPaymaster(true); } else {
195-265: 🛠️ Refactor suggestionRefactor complex paymaster setup with proper async/await.
This complex nested logic still uses
.then()chains and lacks proper error handling, making it difficult to maintain and debug.Extract this into a separate async function with proper error handling:
const setupPaymaster = async (chainId: number, tokens: Token[]) => { try { setQueryString(`chainId=${chainId}&useVp=true`); const paymasterObject = await getAllGaslessPaymasters(chainId, tokens); if (!paymasterObject || paymasterObject.length === 0) { setPaymasterContext(null); setIsPaymaster(false); setFeeType([]); return; } const feeOptions = paymasterObject .map((item) => { const tokenData = tokens.find( (token: Token) => token.contract === item.gasToken.toLowerCase() ); if (tokenData) { return { id: `${item.gasToken}-${item.chainId}-${item.paymasterAddress}-${tokenData.decimals}`, type: 'token', title: tokenData.name, imageSrc: tokenData.logo, chainId: chainNameToChainIdTokensData(tokenData.blockchain), value: tokenData.balance, price: tokenData.price, asset: { ...tokenData, contract: item.gasToken, decimals: tokenData.decimals, }, } as TokenAssetSelectOption; } return undefined; }) .filter((value): value is TokenAssetSelectOption => value !== undefined); if (feeOptions.length > 0) { setFeeAssetOptions(feeOptions); const gasPrice = await getGasPrice(chainId); setGasPrice(gasPrice); setSelectedFeeAsset({ token: feeOptions[0].asset.contract, decimals: feeOptions[0].asset.decimals, }); setSelectedPaymasterAddress(feeOptions[0].id.split('-')[2]); setPaymasterContext({ mode: 'commonerc20', token: feeOptions[0].asset.contract, }); setIsPaymaster(true); } else { setFeeType([]); } } catch (error) { console.error('Failed to setup paymaster:', error); setPaymasterContext(null); setIsPaymaster(false); setFeeType([]); } };Then replace the useEffect content with:
useEffect(() => { if (!walletPortfolio || !selectedAsset) return; const tokens = convertPortfolioAPIResponseToToken(walletPortfolio); setupPaymaster(selectedAsset.chainId, tokens); }, [selectedAsset, walletPortfolio]);
🧹 Nitpick comments (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
131-149: Consider extracting paymaster state to custom hook.The component has accumulated significant paymaster-related state that could be extracted into a reusable custom hook for better organization and testability.
Consider creating a
usePaymasterhook:// hooks/usePaymaster.ts export const usePaymaster = () => { const [isPaymaster, setIsPaymaster] = useState<boolean>(false); const [paymasterContext, setPaymasterContext] = useState<{ mode: string; token?: string; } | null>({ mode: 'sponsor' }); const [selectedPaymasterAddress, setSelectedPaymasterAddress] = useState<string>(''); const [selectedFeeAsset, setSelectedFeeAsset] = useState<{ token: string; decimals: number; }>(); const [feeAssetOptions, setFeeAssetOptions] = useState<TokenAssetSelectOption[]>([]); const [approveData, setApproveData] = useState<string>(''); const [gasPrice, setGasPrice] = useState<string>(); const [feeMin, setFeeMin] = useState<string>(); // Include setup and management logic here return { isPaymaster, paymasterContext, selectedPaymasterAddress, selectedFeeAsset, feeAssetOptions, approveData, gasPrice, feeMin, // setup functions }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)src/services/gasless.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/gasless.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (7)
src/components/Form/AssetSelect/index.tsx (1)
TokenAssetSelectOption(36-41)src/types/api.ts (1)
PortfolioData(723-732)src/apps/the-exchange/reducer/theExchangeSlice.ts (1)
setWalletPortfolio(107-112)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(13-35)src/services/gasless.ts (3)
getAllGaslessPaymasters(23-56)getGasPrice(58-87)GasConsumptions(14-21)src/services/tokensData.ts (2)
Token(11-21)chainNameToChainIdTokensData(237-258)src/components/Form/Select/index.tsx (2)
SelectOption(11-17)Select(65-136)
🪛 Gitleaks (8.26.0)
.env.example
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Cloudflare Pages
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
.env.example
Outdated
|
|
||
| REACT_APP_SWAP_BUTTON_SWITCH="true" | ||
| REACT_APP_PAYMASTER_URL="http://localhost:5050" | ||
| REACT_APP_PAYMASTER_API_KEY=eyJvcmciOiI2NTIzZjY5MzUwOTBmNzAwMDFiYjJkZWIiLCJpZCI6IjMxMDZiOGY2NTRhZTRhZTM4MGVjYjJiN2Q2NDMzMjM4IiwiaCI6Im11cm11cjEyOCJ9 No newline at end of file |
There was a problem hiding this comment.
Replace real API key with placeholder value.
The API key appears to be a real token rather than a placeholder value. Example files should contain placeholder values to avoid accidental exposure of real credentials.
-REACT_APP_PAYMASTER_API_KEY=eyJvcmciOiI2NTIzZjY5MzUwOTBmNzAwMDFiYjJkZWIiLCJpZCI6IjMxMDZiOGY2NTRhZTRhZTM4MGVjYjJiN2Q2NDMzMjM4IiwiaCI6Im11cm11cjEyOCJ9
+REACT_APP_PAYMASTER_API_KEY=your_paymaster_api_key_here📝 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.
| REACT_APP_PAYMASTER_API_KEY=eyJvcmciOiI2NTIzZjY5MzUwOTBmNzAwMDFiYjJkZWIiLCJpZCI6IjMxMDZiOGY2NTRhZTRhZTM4MGVjYjJiN2Q2NDMzMjM4IiwiaCI6Im11cm11cjEyOCJ9 | |
| -REACT_APP_PAYMASTER_API_KEY=eyJvcmciOiI2NTIzZjY5MzUwOTBmNzAwMDFiYjJkZWIiLCJpZCI6IjMxMDZiOGY2NTRhZTRhZTM4MGVjYjJiN2Q2NDMzMjM4IiwiaCI6Im11cm11cjEyOCJ9 | |
| +REACT_APP_PAYMASTER_API_KEY=your_paymaster_api_key_here |
🧰 Tools
🪛 Gitleaks (8.26.0)
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In the .env.example file at line 12, replace the real API key value with a
generic placeholder string such as REACT_APP_PAYMASTER_API_KEY=your_api_key_here
to prevent exposing actual credentials in example files.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
523-535: Fee-balance check still uses the sent asset instead of the fee assetThe balance guard must check the user’s balance of
selectedFeeAsset, not the asset being transferred.
Current logic blocks valid transfers and lets invalid ones slip through.This was called out in a previous review and has not been addressed.
Example patch:
-const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance -); +const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string +]?.find((b) => + addressesEqual(b.token, selectedFeeAsset?.token) || + (selectedFeeAsset?.token === ethers.constants.AddressZero && b.token === null) +)?.balance ?? 0; + +const amountLeft = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset?.decimals ?? 18, +);
854-864: “Gasless” option still maps tocommonerc20Selecting “Gasless” should set
paymasterContext.mode = 'sponsor'and clearselectedFeeAsset.
Keeping it ascommonerc20triggers the whole approval + fee-token flow — the opposite of gasless.Issue was raised earlier but persists.
277-302:setApprovalDataneeds error handling around price fetchNetwork / Coingecko failures will throw and break the hook. Add
try/catchand guard against missing price data.(This is the same concern reported in a previous review and still open.)
🧹 Nitpick comments (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
211-214: Remove stray console.log before mergingA leftover
console.logwill pollute production logs and can leak potentially sensitive user-data.-// eslint-disable-next-line no-console -console.log(paymasterObject);Delete the statement or wrap it in a debug guard.
913-950: Duplicate approval-tx blocks – consider extractingThe same approve-transaction JSX appears three times with nearly identical conditions. Extract it into a small component or helper to DRY up the markup and reduce future maintenance cost.
Also applies to: 1123-1131, 970-978
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)
🧰 Additional context used
🪛 GitHub Actions: vignesha22 has committed new code! 🚀 Checking...
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
[error] 318-318: Prettier formatting error: Insert newline and indentation (prettier/prettier). Run 'prettier --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
141-161: ```shell
#!/bin/bash
set -eShow how queryString is defined and used in the file
rg "queryString" -C3 src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Show how paymasterUrl is referenced around its usages
rg "paymasterUrl" -C3 src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Check if the API key environment variable is used anywhere
rg "REACT_APP_PAYMASTER_API_KEY" -C3 .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Outdated
Show resolved
Hide resolved
| {isPaymaster && | ||
| selectedPaymasterAddress && | ||
| selectedFeeAsset && ( | ||
| <EtherspotTransaction | ||
| to={selectedFeeAsset.token} | ||
| data={approveData} |
There was a problem hiding this comment.
Missing approveData guard – renders empty tx
In the single-transaction path the approval tx is rendered whenever isPaymaster is true, even if approveData is an empty string.
This creates a transaction with no calldata and will revert.
-{isPaymaster &&
- selectedPaymasterAddress &&
- selectedFeeAsset && (
+{isPaymaster &&
+ selectedPaymasterAddress &&
+ selectedFeeAsset &&
+ approveData && (📝 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.
| {isPaymaster && | |
| selectedPaymasterAddress && | |
| selectedFeeAsset && ( | |
| <EtherspotTransaction | |
| to={selectedFeeAsset.token} | |
| data={approveData} | |
| {isPaymaster && | |
| selectedPaymasterAddress && | |
| selectedFeeAsset && | |
| approveData && ( | |
| <EtherspotTransaction | |
| to={selectedFeeAsset.token} | |
| data={approveData} |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 913 to 918, the code renders the EtherspotTransaction component when
isPaymaster is true without checking if approveData is non-empty. This causes a
transaction with empty calldata that will revert. To fix this, add a guard to
ensure approveData is not an empty string before rendering the
EtherspotTransaction component.
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Just a small comment / question
| const handleOnChange = (value: SelectOption) => { | ||
| const tokenOption = feeAssetOptions.filter( | ||
| (option) => option.id === value.id | ||
| )[0] as TokenAssetSelectOption; |
There was a problem hiding this comment.
We're assuming we'll always have array pointer 0 here, is this correct?
There was a problem hiding this comment.
yes thats right if the user has native then it will save else 0
github-actions
bot
temporarily deployed
to
Preview (PRO-3185-Gas_Abstraction)
github-actions
bot
temporarily deployed
to
Preview (PRO-3185-Gas_Abstraction)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (5)
18-18: Technical debt: Mixed ethers and viem dependencies.The codebase is using both ethers and viem libraries, which increases bundle size and complexity. This was flagged in previous reviews but deferred.
Consider creating a migration plan to standardize on viem across the codebase to reduce dependency overhead and improve consistency.
864-874: Critical: "Gasless" mode incorrectly configured as ERC20 paymaster.When "Gasless" is selected, it should use sponsor mode and clear the selected fee asset. Currently it sets
mode: 'commonerc20'which will trigger unnecessary approval logic and request fee tokens from users.if (value.title === 'Gasless') { setPaymasterContext({ - mode: 'commonerc20', - token: selectedFeeAsset?.token, + mode: 'sponsor', }); + setSelectedFeeAsset(undefined); setIsPaymaster(true); } else {
208-281: Refactor complex useEffect with proper error handling and async/await.This useEffect contains deeply nested promise chains, lacks error handling, and has complex logic that should be extracted into separate functions. The current implementation could cause unhandled promise rejections.
Extract the paymaster fetching logic:
useEffect(() => { if (!walletPortfolio) return; const tokens = convertPortfolioAPIResponseToToken(walletPortfolio); if (!selectedAsset) return; setQueryString(`?chainId=${selectedAsset.chainId}`); - getAllGaslessPaymasters(selectedAsset.chainId, tokens).then( - (paymasterObject) => { - if (paymasterObject) { - // ... complex nested logic - } else { - setPaymasterContext(null); - setIsPaymaster(false); - setFeeType([]); - } - } - ); + setupPaymasters(selectedAsset.chainId, tokens); }, [selectedAsset, walletPortfolio]); +const setupPaymasters = async (chainId: number, tokens: Token[]) => { + try { + const paymasterObject = await getAllGaslessPaymasters(chainId, tokens); + if (!paymasterObject) { + resetPaymasterState(); + return; + } + + const feeOptions = mapPaymastersToFeeOptions(paymasterObject, tokens); + if (feeOptions.length === 0) { + setFeeType([]); + return; + } + + await initializePaymasterState(feeOptions, chainId); + } catch (error) { + console.error('Failed to setup paymasters:', error); + resetPaymasterState(); + } +};
530-541: Critical: Balance validation uses wrong token for fee payment.The current implementation checks the balance of the asset being sent (
selectedAsset) instead of the fee asset (selectedFeeAsset). This will incorrectly block valid transactions where users send one token but pay fees with another.-const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance -); -if (!feeMin) return; -if (amountLeft < +feeMin) { +if (!selectedFeeAsset || !feeMin) return; + +const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string +]?.find((b) => + addressesEqual(b.token, selectedFeeAsset.token) || + (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) +)?.balance ?? 0; + +const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, +); + +if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; }
283-308: Add error handling for price fetching and token validation.The
setApprovalDatafunction lacks error handling and doesn't validate that the selected fee asset has a valid price before calculating approval amounts.const setApprovalData = async (gasCost: number) => { + try { if (selectedFeeAsset && gasPrice && gasCost) { const estimatedCost = Number( utils.formatEther(BigNumber.from(gasCost).mul(gasPrice)) ); const costAsFiat = +estimatedCost * nativeAssetPrice; const feeTokenPrice = selectedFeeAsset.tokenPrice; let estimatedCostInToken; - if (feeTokenPrice) { + if (feeTokenPrice && +feeTokenPrice > 0) { estimatedCostInToken = (costAsFiat / +feeTokenPrice).toFixed( selectedFeeAsset.decimals ); setFeeMin(estimatedCostInToken); setApproveData( encodeFunctionData({ abi: erc20Abi, functionName: 'approve', args: [ selectedPaymasterAddress as Address, parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), ], }) ); + } else { + console.error('Invalid fee token price:', feeTokenPrice); + setFeeMin(undefined); + setApproveData(''); } } + } catch (error) { + console.error('Failed to set approval data:', error); + setFeeMin(undefined); + setApproveData(''); + } };
🧹 Nitpick comments (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
140-159: Consider consolidating paymaster state with useReducer.The component has many related paymaster state variables that could benefit from being managed together with useReducer for better state consistency and easier debugging.
type PaymasterState = { isPaymaster: boolean; context: { mode: string; token?: string } | null; selectedAddress: string; selectedFeeAsset?: { token: string; decimals: number; tokenPrice?: string }; feeAssetOptions: TokenAssetSelectOption[]; approveData: string; gasPrice?: string; feeMin?: string; }; const paymasterReducer = (state: PaymasterState, action: PaymasterAction) => { // Handle state transitions }; const [paymasterState, dispatch] = useReducer(paymasterReducer, initialState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
| {isPaymaster && | ||
| selectedPaymasterAddress && | ||
| selectedFeeAsset && ( | ||
| <EtherspotTransaction | ||
| to={selectedFeeAsset.token} | ||
| data={approveData} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure approval transaction guards are consistent.
The batches path checks for approveData (line 982) but the single transaction path doesn't (line 925). This inconsistency could lead to empty approval transactions.
{isPaymaster &&
selectedPaymasterAddress &&
- selectedFeeAsset && (
+ selectedFeeAsset &&
+ approveData && (
<EtherspotTransaction
to={selectedFeeAsset.token}
data={approveData}
/>
)}Also applies to: 980-988
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 923 to 930 and 980 to 988, the single transaction rendering does not check
for the presence of approveData before rendering the EtherspotTransaction
component, unlike the batches path which does. To fix this, add a guard
condition to ensure approveData is truthy before rendering the approval
transaction in both places, preventing empty or invalid approval transactions
from being created.
| getGasPrice(selectedAsset.chainId).then((res) => { | ||
| setGasPrice(res); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for gas price fetching.
The getGasPrice call lacks error handling and could cause unresolved promises if the API call fails.
-getGasPrice(selectedAsset.chainId).then((res) => {
- setGasPrice(res);
-});
+try {
+ const gasPrice = await getGasPrice(selectedAsset.chainId);
+ setGasPrice(gasPrice);
+} catch (error) {
+ console.error('Failed to fetch gas price:', error);
+ setGasPrice(undefined);
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 256 to 258, the getGasPrice call does not handle errors, which can lead to
unresolved promises if the API call fails. Add a catch block to the promise
chain to handle any errors from getGasPrice, and optionally set a default gas
price or show an error message to the user. This will ensure the component
handles failures gracefully without leaving promises unresolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (7)
4-4: Import dependencies properly managedThe addition of viem utilities and ethers BigNumber imports are appropriate for the paymaster implementation. However, note the past review comment about eventually replacing ethers with viem to avoid dual dependencies in the PillarX binary.
Also applies to: 18-18
161-191: Add error handling for wallet portfolio fetchingThe Redux integration for wallet portfolio is implemented correctly, but the error handling could be improved. When
walletPortfolioDataErroroccurs, you're setting a generic error message but not providing specific feedback about what went wrong.Consider enhancing error handling:
if (walletPortfolioDataError) { - console.error(walletPortfolioDataError); - setErrorMessage(t`error.failedWalletPortfolio`); + console.error('Wallet portfolio fetch error:', walletPortfolioDataError); + // Provide more specific error handling based on error type + const errorMessage = walletPortfolioDataError.status === 401 + ? t`error.unauthorized` + : t`error.failedWalletPortfolio`; + setErrorMessage(errorMessage); }
208-282: Extract paymaster setup logic for better maintainabilityThe useEffect for paymaster setup is complex and deeply nested. This logic should be extracted into separate functions for better readability and testability.
Consider refactoring:
const setupPaymasterForAsset = async (asset: AssetSelectOption, tokens: Token[]) => { try { const paymasterData = await getAllGaslessPaymasters(asset.chainId, tokens); if (!paymasterData?.length) { resetPaymasterState(); return; } const feeOptions = buildFeeOptions(paymasterData, tokens); if (feeOptions.length === 0) { setFeeType([]); return; } setFeeAssetOptions(feeOptions); await initializePaymasterSettings(asset.chainId, feeOptions[0]); } catch (error) { console.error('Failed to setup paymaster:', error); resetPaymasterState(); } };
284-309: Improve error handling in setApprovalData functionThe
setApprovalDatafunction lacks proper error handling and validation. The function could fail silently if price data is unavailable.Add comprehensive error handling:
const setApprovalData = async (gasCost: number) => { - if (selectedFeeAsset && gasPrice && gasCost) { + if (!selectedFeeAsset || !gasPrice || !gasCost) { + console.warn('Missing required data for approval calculation'); + return; + } + + try { const estimatedCost = Number( utils.formatEther(BigNumber.from(gasCost).mul(gasPrice)) ); const costAsFiat = +estimatedCost * nativeAssetPrice; const feeTokenPrice = selectedFeeAsset.tokenPrice; - let estimatedCostInToken; - if (feeTokenPrice) { + + if (!feeTokenPrice || +feeTokenPrice <= 0) { + console.error('Invalid or missing fee token price'); + return; + } + + const estimatedCostInToken = (costAsFiat / +feeTokenPrice).toFixed( + selectedFeeAsset.decimals + ); - estimatedCostInToken = (costAsFiat / +feeTokenPrice).toFixed( - selectedFeeAsset.decimals - ); - setFeeMin(estimatedCostInToken); - setApproveData( - encodeFunctionData({ - abi: erc20Abi, - functionName: 'approve', - args: [ - selectedPaymasterAddress as Address, - parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), - ], - }) - ); - } - } + + setFeeMin(estimatedCostInToken); + setApproveData( + encodeFunctionData({ + abi: erc20Abi, + functionName: 'approve', + args: [ + selectedPaymasterAddress as Address, + parseUnits(estimatedCostInToken, selectedFeeAsset.decimals), + ], + }) + ); + } catch (error) { + console.error('Failed to generate approval data:', error); + } };
530-542: Critical: Balance check still uses wrong tokenThe balance validation logic is still incorrect. You're checking
amountLeftfrom the selected asset being sent, but the fee is paid withselectedFeeAsset. This was previously flagged and remains unfixed.The previous fix suggestion should be implemented:
-const amountLeft = +getAmountLeft( - selectedAsset, - amount, - selectedAssetBalance -); -if (!feeMin) return; -if (amountLeft < +feeMin) { +if (!selectedFeeAsset || !feeMin) return; +const feeTokenBalance = accountBalances?.[selectedAsset?.chainId ?? 0]?.[ + accountAddress as string +]?.find((b) => + addressesEqual(b.token, selectedFeeAsset.token) + || (selectedFeeAsset.token === ethers.constants.AddressZero && b.token === null) +)?.balance ?? 0; + +const feeTokenBalanceFormatted = +ethers.utils.formatUnits( + feeTokenBalance, + selectedFeeAsset.decimals, +); + +if (feeTokenBalanceFormatted < +feeMin) { setErrorMessage(t`error.insufficientBalanceForGasless`); setIsSending(false); return; }
865-875: Critical: "Gasless" mode incorrectly configuredThe "Gasless" fee type is still incorrectly setting
mode: 'commonerc20'. This was flagged in previous reviews and remains unfixed. Gasless mode should usesponsormode without fee asset selection.Apply the previously suggested fix:
if (value.title === 'Gasless') { - setPaymasterContext({ - mode: 'commonerc20', - token: selectedFeeAsset?.token, - }); - setIsPaymaster(true); + setPaymasterContext({ mode: 'sponsor' }); + setSelectedFeeAsset(undefined); + setIsPaymaster(true); } else { setPaymasterContext(null); setIsPaymaster(false); }
924-931: Missing approveData guard in single transaction pathThe single transaction rendering path doesn't check for
approveDatapresence, while the batches path does. This inconsistency was previously flagged.Add the missing guard:
{isPaymaster && selectedPaymasterAddress && - selectedFeeAsset && ( + selectedFeeAsset && + approveData && ( <EtherspotTransaction to={selectedFeeAsset.token} data={approveData} /> )}Also applies to: 981-989
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (9)
src/components/Form/AssetSelect/index.tsx (1)
TokenAssetSelectOption(36-41)src/types/api.ts (1)
PortfolioData(731-740)src/apps/the-exchange/reducer/theExchangeSlice.ts (1)
setWalletPortfolio(109-114)src/services/pillarXApiWalletPortfolio.ts (1)
convertPortfolioAPIResponseToToken(22-46)src/services/gasless.ts (2)
getAllGaslessPaymasters(23-56)GasConsumptions(14-21)src/services/tokensData.ts (2)
Token(11-21)chainNameToChainIdTokensData(244-265)src/apps/the-exchange/utils/wrappedTokens.ts (1)
isNativeToken(29-30)src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/components/Form/Select/index.tsx (2)
SelectOption(11-17)Select(65-136)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (3)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
140-159: Well-structured paymaster state managementThe paymaster-related state variables are logically organized and properly typed. The initialization of
paymasterContextwith{ mode: 'sponsor' }is appropriate for the default state.
768-771: Appropriate restriction on batching with paymasterThe logic to prevent batching when paymaster is active is correct. Paymaster validation typically works at the batch level, not individual transactions.
1134-1142: Consistent approval transaction guardsThe regular send modal correctly includes the
approveDataguard for the approval transaction, which is consistent with the batches path. Good defensive programming.
| {selectedAsset && feeType.length > 0 && ( | ||
| <FormGroup> | ||
| <Label>{t`label.feeType`}</Label> | ||
| <Select | ||
| type="token" | ||
| onChange={handleOnChangeFeeAsset} | ||
| options={feeType} | ||
| defaultSelectedId={feeType[0].id} | ||
| /> | ||
| {paymasterContext?.mode === 'commonerc20' && | ||
| feeAssetOptions.length > 0 && ( | ||
| <> | ||
| <Label>{t`label.selectFeeAsset`}</Label> | ||
| <Select | ||
| type="token" | ||
| onChange={handleOnChange} | ||
| options={feeAssetOptions} | ||
| defaultSelectedId={feeAssetOptions[0]?.id} | ||
| /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
UI logic needs refinement for fee type selection
The fee type selection UI is rendered when feeType.length > 0, but the fee asset selection is only shown when paymasterContext?.mode === 'commonerc20'. This creates inconsistent UX when "Gasless" is selected but the mode is incorrectly set.
This will be resolved when the gasless mode configuration issue is fixed. The UI logic is otherwise sound.
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 1075 to 1095, the UI logic conditionally renders the fee asset selection
only when paymasterContext?.mode is 'commonerc20', which causes inconsistent UX
if the mode is incorrectly set while "Gasless" is selected. To fix this, ensure
the paymasterContext mode is correctly configured for gasless mode outside this
component. No changes are needed in the UI logic here as it is sound; focus on
correcting the gasless mode configuration to resolve the inconsistency.
4 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation