feat/PRO-3525/integration-transaction-kit-2 #374
Conversation
WalkthroughReplaces many @etherspot/transaction-kit hooks with a new EtherspotTransactionKit provider and a default useTransactionKit hook, migrates batching/sending to kit.transaction APIs with per-transaction metadata and UserOp polling, removes NFT provider/hooks/UI, updates utils/types, bumps transaction-kit dependency, and updates tests/mocks and test wrappers. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Send Modal / Batches)
participant Hook as useTransactionKit
participant Kit as EtherspotTransactionKit
participant Meta as GlobalTransactionsBatch (meta)
participant Chain as RPC / Bundler
UI->>Hook: useTransactionKit()
Hook-->>UI: { kit, walletAddress, ... }
UI->>Kit: transaction({chainId,to,value,data})
Kit-->>UI: txBuilder
UI->>Kit: name({transactionName}).addToBatch({batchName})
UI->>Meta: setTransactionMetaForName(transactionName, {title, description})
UI->>Kit: estimateBatches({onlyBatchNames:[batchName]})
Kit-->>UI: estimates
UI->>Kit: sendBatches({onlyBatchNames:[batchName]})
Kit-->>UI: userOpHash
UI->>Chain: poll getUserOperationStatus(userOpHash)
Chain-->>UI: status updates (New → Pending → Submitted → OnChain → Confirmed)
sequenceDiagram
participant Exchange as ExchangeAction
participant Hook as useTransactionKit
participant Kit as EtherspotTransactionKit
participant Meta as GlobalTransactionsBatch (meta)
Exchange->>Hook: useTransactionKit()
Hook-->>Exchange: { kit, walletAddress }
Exchange->>Kit: transaction(...).name({txName}).addToBatch({batchName})
Exchange->>Meta: setTransactionMetaForName(txName, {title, description})
Note right of Exchange: Batch modal / estimate/send flows follow Send Modal flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This is a draft, the main implementation is there and working, however all Unit Tests need to be refactored |
Deploying x with
|
| Latest commit: |
064ff31
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b5fbacb5.x-e62.pages.dev |
| Branch Preview URL: | https://feat-pro-3525-integration-tr.x-e62.pages.dev |
github-actions
bot
requested a deployment
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
requested a deployment
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
requested a deployment
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (8)
src/apps/pillarx-app/components/TokensHorizontalTile/TokensHorizontalTile.tsx (2)
35-55: Resize handler uses a stale windowWidth; widths won’t update on resizehandleTokenHorizontalWidthResize closes over the initial windowWidth and never sees the latest viewport size, so token widths won’t recalc after resize.
Fix by reading window.innerWidth inside the handler and removing the captured windowWidth/dependency:
- const windowWidth = window.innerWidth; - useEffect(() => { const handleTokenHorizontalWidthResize = () => { - if (windowWidth >= 1024) { + const width = window.innerWidth; + if (width >= 1024) { setTokenHorizontalWidth(159); - } else if (windowWidth >= 800) { + } else if (width >= 800) { setTokenHorizontalWidth(152); } else { // 102px for width of the TokenInfoHorizontal component 100px + 2px of space setTokenHorizontalWidth(102); } }; handleTokenHorizontalWidthResize(); window.addEventListener('resize', handleTokenHorizontalWidthResize); return () => { window.removeEventListener('resize', handleTokenHorizontalWidthResize); }; - }, [windowWidth]); + }, []);
57-59: createRef inside a function component re-creates the ref each renderThis can break your dimension hook’s stability. Prefer useRef for a stable instance.
Update imports and ref:
-import { createRef, useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; ... - const divRef = createRef<HTMLDivElement>(); + const divRef = useRef<HTMLDivElement>(null);src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx (1)
292-296: Possible runtime crash: unsafe access to data.length without optional chainingtokenDataGraph?.result.data.length omits optional chaining on data. If result exists but data is undefined, this throws.
Patch both occurrences in the template literal:
- className={`flex-1 text-[11px] font-semibold capitalize truncate py-3 rounded ${ - tokenDataGraph?.result.data.length + className={`flex-1 text-[11px] font-semibold capitalize truncate py-3 rounded ${ + tokenDataGraph?.result?.data?.length ? 'hover:bg-green hover:text-dark_grey' : '' - } ${periodFilter === filter && tokenDataGraph?.result.data.length ? 'bg-green text-dark_grey' : 'text-white_grey bg-medium_grey'}`} + } ${periodFilter === filter && tokenDataGraph?.result?.data?.length ? 'bg-green text-dark_grey' : 'text-white_grey bg-medium_grey'}`}src/apps/the-exchange/components/SwapReceiveCard/SwapReceiveCard.tsx (2)
77-119: Effect misses dependencies (selectedToken, chain) and can fail to set receive tokenThe effect uses selectedToken and chain but won’t re-run when they change. If selectedToken becomes available after the initial render, the receive token may never be set.
Apply this diff to include the missing deps and remove the disable comment:
- }, [asset, searchData]); + }, [asset, chain, selectedToken, searchData]);
112-116: Guard against empty search results to avoid undefined accessresult[0] can be undefined, causing an invalid dispatch. Guard before dispatching.
- } else { - dispatch(setReceiveToken(result[0])); - } + } else if (result.length > 0) { + dispatch(setReceiveToken(result[0])); + }src/components/BottomMenuModal/index.tsx (1)
26-33: Attach nodeRef to a DOM element to avoid Transition misbehaviorYou're passing nodeRef to Transition but not assigning it to the rendered DOM node. react-transition-group expects the same ref to be attached to the child; otherwise animations can break and you'll get findDOMNode-related warnings.
Apply this diff:
- <OverflowControlWrapper> + <OverflowControlWrapper ref={modalRef}>src/apps/leaderboard/components/LeaderboardTab/LeaderboardTab.tsx (1)
143-152: Fix potential TypeError when addresses is undefined.Using result.addresses.length inside the index expression will throw if addresses is undefined. Prefer a safe last-element access.
- <div - key={`${result.addresses?.[result.addresses.length - 1]}-${index}`} - className="grid desktop:grid-cols-[70%_15%_15%] tablet:grid-cols-[70%_15%_15%] mobile:grid-cols-[70%_30%] items-center desktop:py-5 tablet:py-5 mobile:py-[5px]" - > + <div + key={`${result.addresses?.slice(-1)?.[0] ?? ''}-${index}`} + className="grid desktop:grid-cols-[70%_15%_15%] tablet:grid-cols-[70%_15%_15%] mobile:grid-cols-[70%_30%] items-center desktop:py-5 tablet:py-5 mobile:py-[5px]" + > ... - walletAddress={ - result.addresses?.[result.addresses.length - 1] || '' - } + walletAddress={result.addresses?.slice(-1)?.[0] || ''}src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (1)
186-191: Don’t dispatch an empty bestOffer on error.When getOffer() fails, it returns {} which is truthy and gets dispatched. This can break downstream logic expecting a valid SwapOffer or undefined.
- const debouncedGetOffer = _.debounce(() => { - getOffer().then((offer) => { - if (offer) dispatch(setBestOffer(offer)); - }); - }, 2000); + const debouncedGetOffer = _.debounce(() => { + getOffer().then((offer) => { + if (offer && Object.keys(offer).length > 0) { + dispatch(setBestOffer(offer)); + } + }); + }, 2000);
🧹 Nitpick comments (66)
src/components/ChainAssetIcon.tsx (1)
7-9: Switch to local TokenListToken type is goodAligns with the PR’s move away from external types. Ensure the local type keeps logoURI optional (see types suggestion) to match asset?.logoURI usage here.
src/apps/pillarx-app/components/TokensHorizontalTile/TokensHorizontalTile.tsx (3)
60-62: Guard against division by 0/NaN when deriving numberTokensHorizontalOn first render tokenHorizontalWidth is 0, causing 0/0 => NaN and an empty slice. A simple guard makes behavior deterministic.
- const numberTokensHorizontal = - Math.floor(dimensions.width / tokenHorizontalWidth) ?? 0; + const numberTokensHorizontal = + tokenHorizontalWidth > 0 + ? Math.floor(dimensions.width / tokenHorizontalWidth) + : 0;Optionally initialize tokenHorizontalWidth to a sensible default (e.g., 102) to reduce layout flicker on first paint.
88-89: Avoid using array index as keyIndex keys can cause unnecessary re-renders and subtle bugs when the list changes.
Consider a stable key from token metadata:
- <TokenInfoHorizontal - key={index} + <TokenInfoHorizontal + key={`${token.symbol}-${compatibleTokenContract?.blockchain ?? index}`}
94-97: URL params should be encoded to avoid malformed URLsSymbols or addresses with unexpected characters can break URLs. Use encodeURIComponent.
- `/token-atlas?${!isZeroAddress(compatibleTokenContract?.address || '') ? `&asset=${compatibleTokenContract?.address}` : `&asset=${token.symbol}`}&blockchain=${compatibleTokenContract?.blockchain}` + `/token-atlas?${!isZeroAddress(compatibleTokenContract?.address || '') ? `&asset=${encodeURIComponent(compatibleTokenContract?.address || '')}` : `&asset=${encodeURIComponent(token.symbol)}`}&blockchain=${encodeURIComponent(compatibleTokenContract?.blockchain || '')}`src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx (1)
210-214: Encode URL parameters to ensure safe navigationAvoid passing raw token or chain values in URLs.
- navigate( - `/the-exchange?${!isZeroAddress(selectedToken?.address || '') ? `&asset=${selectedToken?.address}` : `&asset=${selectedToken?.symbol}`}&blockchain=${chainIdToChainNameTokensData(selectedToken?.chainId)}` - ); + navigate( + `/the-exchange?${!isZeroAddress(selectedToken?.address || '') ? `&asset=${encodeURIComponent(selectedToken?.address || '')}` : `&asset=${encodeURIComponent(selectedToken?.symbol || '')}`}&blockchain=${encodeURIComponent(chainIdToChainNameTokensData(selectedToken?.chainId) || '')}` + );src/apps/pillarx-app/components/TokensVerticalList/TokensVerticalList.tsx (1)
40-40: Simplify the navigation URL construction.The current URL construction has an unnecessary
&at the beginning when using the asset parameter. Consider cleaning this up for better readability.-`/token-atlas?${!isZeroAddress(compatibleTokenContract?.address || '') ? `&asset=${compatibleTokenContract?.address}` : `&asset=${token.symbol}`}&blockchain=${compatibleTokenContract?.blockchain}` +`/token-atlas?${!isZeroAddress(compatibleTokenContract?.address || '') ? `asset=${compatibleTokenContract?.address}` : `asset=${token.symbol}`}&blockchain=${compatibleTokenContract?.blockchain}`src/utils/common.ts (1)
69-71: Add null safety for address substring operations.The
to.substring()calls could throw iftois shorter than expected. Consider adding a length check.-return `${amount} ${selectedAsset.asset.symbol} to ${to.substring(0, 6)}...${to.substring(to.length - 5)}`; +return `${amount} ${selectedAsset.asset.symbol} to ${to.length > 11 ? `${to.substring(0, 6)}...${to.substring(to.length - 5)}` : to}`;src/components/BottomMenuModal/AccountModal.tsx (1)
86-88: Verify chainId parsing logic for edge cases.The chainId parsing logic assumes the format
"eip155:1". Consider adding validation to handle unexpected formats gracefully.-const chainId = Number(contract.chainId.split(':')[1]); // Handle chainId format like "eip155:1" +const chainIdParts = contract.chainId.split(':'); +const chainId = chainIdParts.length === 2 ? Number(chainIdParts[1]) : 0; +if (!chainId) { + console.warn(`Invalid chainId format: ${contract.chainId}`); + return null; +}src/providers/EtherspotTransactionKitProvider.tsx (2)
1-1: Consider the ESLint rule disable carefully.Disabling
react/jsx-no-constructed-context-valuescan lead to performance issues due to unnecessary re-renders. The current implementation does useuseMemoforcontextData, but consider if this ESLint disable is still necessary.Since you're already using
useMemoforcontextData(lines 56-64), the ESLint disable might not be necessary. Consider removing it:-/* eslint-disable react/jsx-no-constructed-context-values */
56-64: Remove setWalletAddress from context data.The
setWalletAddressfunction is included in the context data but doesn't appear to be used by consumers. The AI summary mentions it's "for internal usage", but it's being exposed publicly.const contextData = useMemo( () => ({ walletAddress, - setWalletAddress, kit, activeChainId, setActiveChainId, }), [walletAddress, kit, activeChainId] );src/providers/AccountTransactionHistoryProvider.tsx (1)
1-3: Consider removing ESLint disable comments if no longer needed.The file has two ESLint disable comments that may no longer be necessary after the refactoring. The
no-restricted-syntaxrule is typically for loop restrictions, andreact/jsx-no-constructed-context-valuesis for preventing inline object creation in context providers - but you're usinguseMemowhich already addresses this concern.Consider removing these if they're no longer needed:
-/* eslint-disable no-restricted-syntax */ -/* eslint-disable react/jsx-no-constructed-context-values */ import React, { createContext, useMemo, useState } from 'react';src/apps/token-atlas/index.tsx (1)
64-65: Consider consistent naming between walletAddress and accountAddress.While aliasing
walletAddressasaccountAddressworks, consider using consistent naming across the codebase to avoid confusion. The transaction kit returnswalletAddress, so using that name consistently might be clearer.- const { walletAddress: accountAddress } = useTransactionKit(); + const { walletAddress } = useTransactionKit();Then update the references:
- if (accountAddress) { + if (walletAddress) { recordPresence({ - address: accountAddress, + address: walletAddress,src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (1)
67-68: Consider consistent naming for wallet/account address variables.Similar to the token-atlas file, there's an inconsistency in naming where
walletAddressis aliased asaccountAddress. For better maintainability and clarity, consider using consistent naming throughout the codebase.Either use
walletAddressconsistently:- const { walletAddress: accountAddress } = useTransactionKit(); + const { walletAddress } = useTransactionKit();Or if
accountAddressis the preferred convention in this module, consider documenting why the alias is necessary.src/apps/pillarx-app/components/EditorialTile/EditorialTile.tsx (2)
85-95: Expose a stable test selector on the TileContainer.Tests use queryByTestId('editorial-tile'), but the component currently sets id, not data-testid. Add a data-testid to avoid brittle DOM queries in tests and improve consistency.
Apply this diff:
- <TileContainer - id="editorial-tile" + <TileContainer + id="editorial-tile" + data-testid="editorial-tile" className="flex-col desktop:p-10 desktop:pt-[30px] tablet:p-5 mobile:p-0 mobile:bg-[#1F1D23]" >
90-94: Harden external link by adding noopener.For target="_blank", include noopener to prevent access to window.opener.
- rel="noreferrer" + rel="noopener noreferrer"src/apps/deposit/components/SendAsset/SendAsset.tsx (2)
80-84: Fix typos in user-facing messages.Correct “succesfully/succesful/occured” to “successfully/successful/occurred”.
- ? `The transaction has succesfully been sent to your external wallet with the transaction hash: ${txHash}. Please check your external wallet to follow the transaction status.` - : 'The transaction has not been succesful. Please check your external wallet to check the transaction status.' + ? `The transaction has successfully been sent to your external wallet with the transaction hash: ${txHash}. Please check your external wallet to follow the transaction status.` + : 'The transaction has not been successful. Please check your external wallet to check the transaction status.'- 'An error occured while sending the transaction to your external wallet. Please try again' + 'An error occurred while sending the transaction to your external wallet. Please try again'- ? `The transaction has succesfully been sent to your external wallet with the transaction hash: ${txHash}. Please check your external wallet to follow the transaction status.` - : 'The transaction has not been succesful. Please check your external wallet to check the transaction status.' + ? `The transaction has successfully been sent to your external wallet with the transaction hash: ${txHash}. Please check your external wallet to follow the transaction status.` + : 'The transaction has not been successful. Please check your external wallet to check the transaction status.'- 'An error occured while sending the transaction to your external wallet. Please try again' + 'An error occurred while sending the transaction to your external wallet. Please try again'Also applies to: 92-93, 112-115, 123-124
188-193: Improve UX by shortening the displayed wallet address.Showing the full address is noisy in UI. Consider showing a shortened version.
- manually transfer this NFT to your PillarX wallet address:{' '} - {walletAddress} + manually transfer this NFT to your PillarX wallet address:{' '} + {walletAddress + ? `${walletAddress.slice(0, 6)}…${walletAddress.slice(-4)}` + : ''}src/apps/pillarx-app/components/EditorialTile/test/EditorialTile.test.tsx (1)
231-231: Align test selector with component markup to avoid false positives.These tests use queryByTestId('editorial-tile'), but the component sets an id, not data-testid. Either:
- Add data-testid="editorial-tile" to EditorialTile’s TileContainer (recommended; see component comment), and add a positive assertion in one test to ensure it renders when data is valid, or
- Change these queries to use document.getElementById('editorial-tile') if you prefer to keep using id.
This will make the “renders nothing” tests meaningful rather than vacuously passing.
Also applies to: 249-249, 269-269, 279-279
src/components/BottomMenu/index.tsx (2)
23-23: Switch to safer access pattern when kit isn’t readyDirectly assuming
kit.getState()is available can crash during early renders or in tests if the hook is mocked or provider isn’t mounted. Use optional chaining and a default to avoid runtime errors.-import useTransactionKit from '../../hooks/useTransactionKit'; +import useTransactionKit from '../../hooks/useTransactionKit';
40-42: Guard against undefined kit state and compute in one placeIf
kitorgetState()is not ready, destructuringbatcheswill throw. Consolidate the computation and default to an empty object.- const { kit } = useTransactionKit(); - const { batches } = kit.getState(); - const batchCount = Object.keys(batches).length; + const { kit } = useTransactionKit(); + const batchCount = Object.keys(kit?.getState()?.batches ?? {}).length;src/apps/token-atlas/components/HeaderSearch/HeaderSeach.tsx (1)
26-37: Avoid recording presence without an addressGuard the analytics event to avoid sending an undefined/empty address.
- const handleSearchOpen = async () => { + const handleSearchOpen = async () => { dispatch(setIsSearchTokenModalOpen(true)); - recordPresence({ - address: accountAddress, - action: 'app:tokenAtlas:searchOpen', - value: 'SEARCH_OPEN', - }); + if (accountAddress) { + recordPresence({ + address: accountAddress, + action: 'app:tokenAtlas:searchOpen', + value: 'SEARCH_OPEN', + }); + } };src/components/AppsList.tsx (2)
30-35: Avoid double-calling useAllowedAppsThe hook is invoked twice; consolidate to a single call to prevent duplicate subscriptions and ensure consistency.
- const { setIsAnimated } = useAllowedApps(); - const { isLoading: isLoadingAllowedApps, allowed } = useAllowedApps(); + const { setIsAnimated, isLoading: isLoadingAllowedApps, allowed } = useAllowedApps();
80-91: Guard presence event and drop lint suppression via template literal
- Don’t send analytics if address is missing.
- Prefer a template literal to avoid the eslint suppression.
recordPresence({ - address: accountAddress, + address: accountAddress, action: 'appOpened', value: appId, }); setIsAnimated(true); - // eslint-disable-next-line prefer-template - navigate('/' + appId); + if (accountAddress) { + recordPresence({ address: accountAddress, action: 'appOpened', value: appId }); + } + navigate(`/${appId}`);src/apps/pillarx-app/components/ReceiveModal/ReceiveModal.tsx (2)
28-29: Naming consistencyAliasing
walletAddresstoaccountAddressis fine. Consider standardizing on a single name across the app to reduce cognitive overhead.
95-104: Don’t show “copied” for an empty addressCurrently CopyToClipboard will copy an empty string if no address is available and still show the copied checkmark. Guard the handler.
- <CopyToClipboard - text={accountAddress || ''} - onCopy={() => setCopied(true)} - > + <CopyToClipboard + text={accountAddress || ''} + onCopy={() => accountAddress && setCopied(true)} + >Optionally, make the row non-interactive when there’s no address:
- <div className="flex items-center justify-between p-3 bg-lighter_container_grey rounded-[10px] mb-6 max-w-full cursor-pointer"> + <div className={`flex items-center justify-between p-3 bg-lighter_container_grey rounded-[10px] mb-6 max-w-full ${accountAddress ? 'cursor-pointer' : 'cursor-not-allowed opacity-60'}`}>src/apps/leaderboard/index.tsx (1)
79-93: Harden null-safety around migrationQuery shape.Guard against absent or empty result arrays to avoid potential undefined access in edge cases.
Apply:
- if (!walletAddress || !migrationQuery.data) { + if (!walletAddress || !migrationQuery.data?.result?.length) { dispatch(setIsUserInMigrationData(false)); return; } - const userInMigrationData = migrationQuery.data.result.some((result) => + const userInMigrationData = migrationQuery.data.result?.some((result) => result.pxAddresses.some( (address) => address.toLowerCase() === walletAddress.toLowerCase() ) - ); + ) ?? false;src/components/BottomMenuModal/HistoryModal/TransactionsHistory.tsx (1)
36-43: Fix typo in isHistorySucess alias.Minor spelling issue; rename to isHistorySuccess for clarity and consistency.
- isSuccess: isHistorySucess, + isSuccess: isHistorySuccess, ... - {(!allTransactions.length && !isHistoryLoading && isHistorySucess) || + {(!allTransactions.length && !isHistoryLoading && isHistorySuccess) ||Also applies to: 112-119
src/apps/the-exchange/components/SelectDropdown/SelectDropdown.tsx (1)
78-83: Guard analytics calls when address is absent.recordPresence with an undefined address can pollute metrics. Add a truthy check.
- recordPresence({ - address: accountAddress, - action: 'app:theExchange:sourceChainSelect', - value: { chainId: option }, - }); + if (accountAddress) { + recordPresence({ + address: accountAddress, + action: 'app:theExchange:sourceChainSelect', + value: { chainId: option }, + }); + } ... - recordPresence({ - address: accountAddress, - action: 'app:theExchange:destinationChainSelect', - value: { chainId: option }, - }); + if (accountAddress) { + recordPresence({ + address: accountAddress, + action: 'app:theExchange:destinationChainSelect', + value: { chainId: option }, + }); + }Also applies to: 91-96
src/apps/token-atlas/components/TokensSearchInput/TokensSearchInput.tsx (2)
77-105: Broaden effect deps to include selectedChain.chainId.The effect filters results by chainId but only depends on chainName; include chainId to avoid stale filtering in edge cases.
- }, [searchData, debouncedSearchText, selectedChain.chainName]); + }, [searchData, debouncedSearchText, selectedChain.chainName, selectedChain.chainId]);
107-116: Guard presence event when address is missing.Avoid emitting analytics with undefined addresses.
- if (debouncedSearchText !== '') { + if (debouncedSearchText !== '' && accountAddress) { recordPresence({ address: accountAddress, action: 'app:tokenAtlas:search', value: { debouncedSearchText }, }); }src/apps/the-exchange/utils/sentry.ts (3)
21-33: Pass walletAddress into logging APIs to avoid “unknown_wallet_address” in SentryRight now these utilities always tag “unknown_wallet_address” unless callers supply it via the tags/extra. Expose an optional walletAddress param (like other helpers) and use it with a fallback. This keeps the API flexible and avoids forcing React-hook usage in non-React contexts.
Apply these diffs:
-export const logExchangeEvent = ( - message: string, - level: Sentry.SeverityLevel = 'info', - extra?: Record<string, unknown>, - tags?: Record<string, string> -) => { - const walletAddress = fallbackWalletAddressForLogging(); +export const logExchangeEvent = ( + message: string, + level: Sentry.SeverityLevel = 'info', + extra?: Record<string, unknown>, + tags?: Record<string, string>, + walletAddress?: string +) => { + const addr = walletAddress || fallbackWalletAddressForLogging(); Sentry.withScope((scope) => { scope.setLevel(level); - scope.setTag('wallet_address', walletAddress); + scope.setTag('wallet_address', addr); scope.setTag('app_module', 'the-exchange');-export const addExchangeBreadcrumb = ( - message: string, - category: string = 'exchange', - data?: Record<string, unknown>, - level: Sentry.SeverityLevel = 'info' -) => { - const walletAddress = fallbackWalletAddressForLogging(); +export const addExchangeBreadcrumb = ( + message: string, + category: string = 'exchange', + data?: Record<string, unknown>, + level: Sentry.SeverityLevel = 'info', + walletAddress?: string +) => { + const addr = walletAddress || fallbackWalletAddressForLogging(); Sentry.addBreadcrumb({ message, category, level, data: { ...data, - wallet_address: walletAddress, + wallet_address: addr, app_module: 'the-exchange', }, });Also applies to: 225-244
49-61: Add walletAddress param to error logger for consistencyMirror the change in logExchangeEvent so errors can tag the real wallet when available.
-export const logExchangeError = ( - error: Error | string, - extra?: Record<string, unknown>, - tags?: Record<string, string> -) => { - const walletAddress = fallbackWalletAddressForLogging(); +export const logExchangeError = ( + error: Error | string, + extra?: Record<string, unknown>, + tags?: Record<string, string>, + walletAddress?: string +) => { + const addr = walletAddress || fallbackWalletAddressForLogging(); Sentry.withScope((scope) => { scope.setLevel('error'); - scope.setTag('wallet_address', walletAddress); + scope.setTag('wallet_address', addr); scope.setTag('app_module', 'the-exchange'); scope.setTag('error_type', 'exchange_error');
195-199: Hook usage inside utils file is OK; small nit on PII/formattingUsing the hook only within a React hook exported from this module is fine. Consider masking the wallet address (e.g., 0x1234…abcd) before tagging to reduce PII exposure in Sentry.
src/apps/the-exchange/components/SwapReceiveCard/SwapReceiveCard.tsx (3)
139-146: Clamp negative balance after subtracting deployment costIf deploymentCost exceeds the native token balance, adjustedBalance becomes negative. Clamp to zero to prevent odd UX downstream.
- return adjustedBalance; + return Math.max(0, adjustedBalance);
121-137: Optional: Avoid state updates after unmount in async cost fetchIf the component unmounts or account/swapToken changes mid-flight, you may update stale state. Add a cancellation guard.
useEffect(() => { - const getDeploymentCost = async () => { + let cancelled = false; + const getDeploymentCost = async () => { if (!accountAddress || !swapToken?.blockchain) return; setIsDeploymentCostLoading(true); const cost = await getWalletDeploymentCost({ accountAddress, chainId: chainNameToChainIdTokensData(swapToken.blockchain), }); - setDeploymentCost(cost); - setIsDeploymentCostLoading(false); + if (!cancelled) { + setDeploymentCost(cost); + setIsDeploymentCostLoading(false); + } }; getDeploymentCost(); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [accountAddress, swapToken]); + return () => { + cancelled = true; + }; + }, [accountAddress, swapToken]);
93-99: Optional: Avoid direct window access to keep SSR-friendlyIf this component ever renders in SSR, direct window usage will crash. Gate by typeof window !== 'undefined'.
src/hooks/useTransactionKit.tsx (1)
13-14: Tighten narrowing: avoid optional chaining after null-checkAfter throwing on null, context is non-null; returning context.data improves type inference.
- return context?.data; + return context.data;src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx (2)
188-199: Skip presence recording when walletAddress is absentAvoid sending undefined addresses to the presence API.
- useEffect(() => { - if (debouncedSearchText !== '') { - recordPresence({ - address: accountAddress, - action: 'app:theExchange:search', - value: { debouncedSearchText }, - }); - } + useEffect(() => { + if (debouncedSearchText !== '' && accountAddress) { + recordPresence({ + address: accountAddress, + action: 'app:theExchange:search', + value: { debouncedSearchText }, + }); + }
136-153: Optional: Avoid running two searches for “contract-like” queriesWhen debouncedSearchText.length > 40 you compute an exact-match result, but then run a second broader Fuse search that overwrites it. Consider returning early after dispatching exact results to preserve intent and avoid extra work.
src/components/BottomMenuModal/index.tsx (2)
19-24: Use last valid index for visibility to avoid content flicker on closeYou persist lastValidActiveIndex for transform but isContentVisible still keys off activeIndex directly. When activeIndex is null during exit, content disappears while the container animates, causing a blank flash.
Apply this diff:
- $activeIndex={activeIndex ?? lastValidActiveIndex.current} + $activeIndex={activeIndex ?? lastValidActiveIndex.current} ... - isContentVisible={activeIndex === index} + isContentVisible={(activeIndex ?? lastValidActiveIndex.current) === index}Also applies to: 36-37, 41-47
38-45: Remove redundant inner key propYou already set key on ModalContent. The duplicate key on Modal is unnecessary noise.
Apply this diff:
- <ModalContent key={index}> - <Modal - key={`${index}`} + <ModalContent key={index}> + <Modalsrc/apps/token-atlas/components/TokensSlider/TokensSlider.tsx (1)
74-80: Guard presence recording when address is unavailableAvoid sending presence with an undefined address. Cheap guard inside the debounced callback prevents noisy analytics.
Apply this diff:
const debouncedTokenTrendingScroll = _.debounce(() => { - recordPresence({ - address: accountAddress, - action: 'app:tokenAtlas:trendingScroll', - value: 'TRENDING_SCROLL', - }); + if (!accountAddress) return; + recordPresence({ + address: accountAddress, + action: 'app:tokenAtlas:trendingScroll', + value: 'TRENDING_SCROLL', + }); }, 2000);src/apps/token-atlas/components/SelectChainDropdown/SelectChainDropdown.tsx (1)
72-77: Guard presence recording when wallet is not connected.If walletAddress is undefined, this may write an invalid presence record. Skip calling the mutation when there’s no address.
- recordPresence({ - address: accountAddress, - action: 'app:tokenAtlas:chainSelect', - value: { chainId: option }, - }); + if (accountAddress) { + recordPresence({ + address: accountAddress, + action: 'app:tokenAtlas:chainSelect', + value: { chainId: option }, + }); + }src/apps/leaderboard/components/LeaderboardTab/LeaderboardTab.tsx (1)
58-60: Make wallet address matching case-insensitive to avoid checksum-case mismatches.If API addresses are checksummed and kit returns lowercased (or vice versa), includes() may fail. Normalize both sides.
Example:
const normalized = walletAddress?.toLowerCase(); const myRankData = normalized ? data.find((r) => r.addresses?.some((a) => a?.toLowerCase() === normalized)) : undefined;src/apps/pillarx-app/components/PortfolioOverview/test/PortfolioOverview.test.tsx (1)
87-88: Avoid hard-coding the percentage string; derive from mock data.This keeps the test resilient to formatting logic changes.
- expect(screen.getByText('1.00%')).toBeInTheDocument(); + const pct = + `${mockData.data.total_pnl_history['24h'].percentage_change.toFixed(2)}%`; + expect(screen.getByText(pct)).toBeInTheDocument();src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (1)
186-191: Optional: memoize debouncedGetOffer to avoid re-creating it each render.Not critical, but using useMemo/useCallback reduces churn and improves readability.
Example:
const getOfferCb = useCallback(getOffer, [amountSwap, swapToken, receiveToken, isAboveLimit, walletAddress]); const debouncedGetOffer = useMemo( () => _.debounce(() => { getOfferCb().then((offer) => { if (offer && Object.keys(offer).length > 0) dispatch(setBestOffer(offer)); }); }, 2000), [getOfferCb], ); useEffect(() => () => debouncedGetOffer.cancel(), [debouncedGetOffer]);src/apps/the-exchange/components/SelectToken/test/SelectToken.test.tsx (2)
21-23: Remove duplicate vi.clearAllMocks(); keep it in one place.You’re clearing mocks in both beforeEach and afterEach. One is sufficient.
- beforeEach(() => { - vi.clearAllMocks(); - }); ... - afterEach(() => { - vi.clearAllMocks(); - }); + afterEach(() => { + vi.clearAllMocks(); + });Also applies to: 89-91
83-87: Prefer userEvent for interactions over fireEvent.userEvent.click better emulates user behavior and reduces brittle tests.
Example:
import userEvent from '@testing-library/user-event'; const selectTokenElement = screen.getByText('Ether'); await userEvent.click(selectTokenElement); expect(mockOnClick).toHaveBeenCalledTimes(1);src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx (1)
129-139: Test assertions could be more comprehensiveThe test currently only verifies that the input element exists and can receive input. Consider also verifying that the Redux state is updated correctly when the input changes.
// The input should be present but may not have the expected initial value due to test store issues const input = screen.getByTestId('enter-amount-input'); expect(input).toBeInTheDocument(); // Test that the input can receive user input fireEvent.change(input, { target: { value: '0.5' } }); // Wait for the input value to be updated await waitFor(() => { expect(input).toHaveValue(0.5); + // Also verify that the Redux state was updated if needed + // This would require properly setting up the store mock });src/utils/blockchain.ts (1)
16-18: Avoid cross-layer import: move isNativeToken to a shared utils moduleutils/blockchain now depends on apps/the-exchange/utils/wrappedTokens, which inverts layering and couples global utilities to an app feature. Extract isNativeToken (and its constants) into a shared module under src/utils or src/constants and import it from there.
src/apps/leaderboard/components/PxPointsSummary/tests/PxPointsSummary.test.tsx (1)
13-14: Prefer vi.mocked for type-safe hook mocks (avoid casting to unknown)Casting to unknown as Mock works but is brittle. Use vi.mocked to get a properly typed mocked function and drop the extra type import.
Apply this diff:
-import type { Mock } from 'vitest'; +// no extra type import needed import PxPointsSummary from '../PxPointsSummary'; import useTransactionKit from '../../../../../hooks/useTransactionKit'; @@ -vi.mock('../../../../../hooks/useTransactionKit'); +vi.mock('../../../../../hooks/useTransactionKit'); @@ - const useTransactionKitMock = useTransactionKit as unknown as Mock; + const useTransactionKitMock = vi.mocked(useTransactionKit);Also applies to: 44-49
src/providers/__tests__/GlobalTransactionsBatchProvider.test.tsx (1)
66-89: Consider adding a test for clearing/unsetting metadataYou validate overwriting the same key (great). Consider also testing how the API behaves when “clearing” metadata (e.g., setting undefined or deleting the key) if that’s a supported scenario. It prevents regressions in UIs that expect a removal flow.
If supported, I can add a small test covering “unset” semantics and update the provider accordingly.
src/components/Form/AssetSelect/index.tsx (2)
185-203: UI simplification opportunity: remove Tabs wrapper since there’s only one tabWith NFTs removed, Tabs/TabList/Tab add overhead for a single static tab. You can replace it with a simple section header, which reduces DOM and complexity.
Apply this diff to inline a lightweight label:
- <CssVarsProvider defaultMode="dark"> - <Tabs sx={{ bgcolor: 'transparent', mb: 1 }} value={0}> - <TabList - disableUnderline - sx={{ - p: 0.5, - gap: 0.5, - borderRadius: 'sm', - bgcolor: 'background.level1', - [`& .${tabClasses.root}[aria-selected="true"]`]: { - boxShadow: 'sm', - bgcolor: 'background.surface', - }, - }} - tabFlex={1} - > - <Tab disableIndicator>{t`label.tokens`}</Tab> - </TabList> - </Tabs> - </CssVarsProvider> + <CssVarsProvider defaultMode="dark"> + <div style={{ marginBottom: 8 }}>{t`label.tokens`}</div> + </CssVarsProvider>
206-210: Nit: expose a data-testid for the select to simplify testsIf you’re frequently testing this component, adding data-testid to the Select (e.g., data-testid="asset-select") can make tests more resilient.
- <Select + <Select + data-testid="asset-select" options={availableAssetsInWallet} type="token" defaultSelectedId={defaultSelectedId} isLoadingOptions={isLoadingAssets}src/apps/token-atlas/components/TokenGraph/test/TokenGraph.test.tsx (1)
59-74: Optionally extend TestWrapper to accept preloaded state to avoid custom Provider wiringYou create a custom store only for the “no price history” case. Consider enhancing TestWrapper to accept an optional store or preloadedState so all tests share identical provider stacks.
I can draft an overload like:
- TestWrapper({ children, store?: Store, preloadedState?: Partial })
and switch this test to use it for consistency.
Also applies to: 76-82
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (2)
260-273: Redundant bigint conversion logicThe bigint conversion logic has redundant branches that could be simplified for better readability.
Simplify the conversion logic:
-// Handle bigint conversion properly -let bigIntValue: bigint; -if (typeof value === 'bigint') { - // If value is already a native bigint, use it directly - bigIntValue = value; -} else if (value) { - // If value exists but is not a bigint, convert it - bigIntValue = BigNumber.from(value).toBigInt(); -} else { - // If value is undefined/null, use 0 - bigIntValue = BigInt(0); -} +// Handle bigint conversion properly +const bigIntValue = typeof value === 'bigint' + ? value + : BigInt(value?.toString() || '0');
299-326: Consider extracting transaction naming logicThe transaction naming logic with string concatenation is repeated and could be extracted into a helper function for better maintainability.
Extract the naming logic:
// Add this helper function at the top of the file const generateTransactionName = (chainId: number, data: string | undefined): string => { return `tx-${chainId}-${data || ''}`; }; const generateBatchName = (chainId: number): string => { return `batch-${chainId}`; };Then use it in the code:
-const chainId = - chainNameToChainIdTokensData(swapToken?.blockchain) || 0; -const transactionName = `tx-${chainId}-${data}`; -const batchName = `batch-${chainId}`; +const chainId = chainNameToChainIdTokensData(swapToken?.blockchain) || 0; +const transactionName = generateTransactionName(chainId, data?.toString()); +const batchName = generateBatchName(chainId);src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (1)
324-470: Complex UserOp status monitoring could benefit from state machine patternThe UserOp status monitoring logic has multiple status checks and branching conditions that are hard to follow. This could lead to maintenance issues and potential bugs.
Consider implementing a state machine pattern to handle the different UserOp statuses more cleanly:
enum UserOpState { PENDING = 'Pending', NEW = 'New', SUBMITTED = 'Submitted', ONCHAIN = 'OnChain', REVERTED = 'Reverted', FAILED = 'Failed' } const STATUS_TRANSITIONS = { [UserOpState.NEW]: { uiStatus: 'Sending', continue: true }, [UserOpState.PENDING]: { uiStatus: 'Sending', continue: true }, [UserOpState.SUBMITTED]: { uiStatus: 'Sent', continue: true }, [UserOpState.ONCHAIN]: { uiStatus: 'Confirmed', continue: false }, [UserOpState.REVERTED]: { uiStatus: 'Failed', continue: false }, }; const handleUserOpStatus = (status: string, attempts: number, maxAttempts: number) => { const transition = STATUS_TRANSITIONS[status as UserOpState]; if (transition) { setUserOpStatus(transition.uiStatus); return transition.continue || attempts < maxAttempts; } return false; };src/test-utils/testUtils.tsx (2)
110-129: Consider MemoryRouter over BrowserRouter in tests
BrowserRouterdepends on full DOM/history APIs and can be harder to control in tests.MemoryRouteris purpose-built for test environments and lets you defineinitialEntries.Example:
-import { BrowserRouter } from 'react-router-dom'; +import { MemoryRouter } from 'react-router-dom'; - <Provider store={testStore}> - <BrowserRouter> + <Provider store={testStore}> + <MemoryRouter initialEntries={['/']}> ... - </BrowserRouter> + </MemoryRouter> </Provider>
131-151: Reduce wrapper duplication with a small factoryThe five wrappers share the same structure with minor differences. A thin factory avoids drift and eases future changes to provider stacks.
Sketch:
type WrapperOpts = { withGlobalBatch?: boolean; withTheme?: boolean; withLang?: boolean; store?: typeof testStore }; export const makeTestWrapper = (opts: WrapperOpts = {}) => { const { withGlobalBatch = false, withTheme = true, withLang = true, store = testStore } = opts; return ({ children }: { children: React.ReactNode }) => ( <Provider store={store}> <MemoryRouter initialEntries={['/']}> {withTheme ? <ThemeProvider theme={defaultTheme}>{withLang ? <LanguageProvider>{withGlobalBatch ? <GlobalTransactionsBatchProvider>{children}</GlobalTransactionsBatchProvider> : children}</LanguageProvider> : children}</ThemeProvider> : children} </MemoryRouter> </Provider> ); }; // Usage export const ExchangeTestWrapper = makeTestWrapper({ withGlobalBatch: true });Also applies to: 153-169, 170-186, 187-203, 204-219
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (3)
127-132: Duplicate mocking of useTransactionKit; consolidate to setupTests
useTransactionKitis already mocked globally insrc/test-utils/setupTests.ts. Re-mocking it here risks divergence and unexpected behavior due to hoisting order.Apply this diff to rely on the global mock:
-// Mock useTransactionKit hook -vi.mock('../../../../hooks/useTransactionKit', () => ({ - default: vi.fn(() => ({ - walletAddress: '0x1234567890123456789012345678901234567890', - })), -}));
153-164: Snapshot test adds little value for this component; favor targeted assertionsSnapshot tests for large UI trees are brittle. You already have semantic assertions below. Consider removing the snapshot or replacing it with a few focused checks on critical DOM nodes/labels to reduce churn.
98-125: Local tokensData mock duplicates setup-level mocksYou mock
tokensDatahere and also insetupTests.ts. Keeping both increases maintenance cost. Prefer keeping the test-local mock only if these specific mappings are necessary for this suite; otherwise rely on the setup mock.src/test-utils/setupTests.ts (3)
304-323: Remove console logs from react-chartjs-2 mockConsole output in tests is noisy and makes CI logs harder to parse.
Apply this diff:
-vi.mock('react-chartjs-2', () => { - console.log('react-chartjs-2 mock applied'); - return { - Line: ({ data, options, ...props }: any) => { - console.log('Line component rendered with props:', props); +vi.mock('react-chartjs-2', () => { + return { + Line: ({ data, options, ...props }: any) => { // Create a mock canvas element using React.createElement const React = require('react'); const canvas = React.createElement('canvas', { 'data-testid': props['data-testid'] || 'price-graph', width: '300', height: '150', role: 'img', ...props, }); return canvas; }, }; });
66-69: Viem wallet client setup appears unused and adds overheadThe imports and creation of
randomWallet/providerare not referenced. They also pull in network config and could complicate test environments.Apply this diff to remove the unused code:
-import { createWalletClient, http } from 'viem'; -import { privateKeyToAccount } from 'viem/accounts'; -import { goerli } from 'viem/chains'; @@ -const randomWallet = privateKeyToAccount( - `0x${crypto.getRandomValues(new Uint8Array(32)).reduce((acc, byte) => acc + byte.toString(16).padStart(2, '0'), '')}` -); -const provider = createWalletClient({ - account: randomWallet, - chain: goerli, - transport: http('http://localhost:8545'), -});Also applies to: 175-183
49-61: Crypto polyfill subtle stub may be insufficientSome code paths may call
crypto.subtle.digestor other WebCrypto methods. Currentlysubtleis an empty object. If you see runtime errors, consider adding a minimaldigestmock.Example:
- globalThis.crypto = { + globalThis.crypto = { getRandomValues: (arr: Uint8Array) => { const bytes = crypto.randomBytes(arr.length); arr.set(bytes); return arr; }, - subtle: {} as any, + subtle: { + digest: vi.fn(async (_algo: string, data: ArrayBuffer) => data), + } as any, } as any;
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx
Outdated
Show resolved
Hide resolved
src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 27
🔭 Outside diff range comments (17)
src/apps/pillarx-app/components/TokensHorizontalTile/TokensHorizontalTile.tsx (1)
94-97: Fix asset fallback when address is missing; avoid appending blockchain=undefinedCurrent logic uses
isZeroAddress(compatibleTokenContract?.address || '')which treats an empty string as “not zero,” resulting in&asset=with an empty value. Also,blockchain=${compatibleTokenContract?.blockchain}can produceblockchain=undefined. Guard both cases.Apply this diff:
- navigate( - `/token-atlas?${!isZeroAddress(compatibleTokenContract?.address || '') ? `&asset=${compatibleTokenContract?.address}` : `&asset=${token.symbol}`}&blockchain=${compatibleTokenContract?.blockchain}` - ) + navigate( + `/token-atlas?${compatibleTokenContract?.address && !isZeroAddress(compatibleTokenContract.address) ? `&asset=${compatibleTokenContract.address}` : `&asset=${token.symbol}`}${compatibleTokenContract?.blockchain ? `&blockchain=${compatibleTokenContract.blockchain}` : ''}` + )Optional: build the query with URLSearchParams for clarity.
src/apps/pillarx-app/components/TokensVerticalList/TokensVerticalList.tsx (1)
39-41: Use correct asset fallback when address is absent; guard blockchain paramSame edge case as the horizontal tile: empty or undefined address should fall back to symbol; avoid appending
blockchain=undefined.Apply this diff:
- navigate( - `/token-atlas?${!isZeroAddress(compatibleTokenContract?.address || '') ? `&asset=${compatibleTokenContract?.address}` : `&asset=${token.symbol}`}&blockchain=${compatibleTokenContract?.blockchain}` - ) + navigate( + `/token-atlas?${compatibleTokenContract?.address && !isZeroAddress(compatibleTokenContract.address) ? `&asset=${compatibleTokenContract.address}` : `&asset=${token.symbol}`}${compatibleTokenContract?.blockchain ? `&blockchain=${compatibleTokenContract.blockchain}` : ''}` + )src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx (1)
136-152: Double-dispatch overwrites exact-match results in swap searchWhen debouncedSearchText.length > 40, you dispatch exact-match results, then immediately rebuild Fuse and dispatch the generic results, overwriting the first dispatch. Wrap the generic search in an else block (or return after the first dispatch) and apply chain filtering consistently in both branches.
Apply this diff to fix the overwrite and avoid redundant work:
@@ - if (debouncedSearchText.length > 40) { - const fuse = new Fuse(tokensWithBalances, { - keys: ['name', 'symbol', 'contract'], // Fields to search in - threshold: 0.2, // Allow some fuzziness for queries that are not contract like - minMatchCharLength: 3, - useExtendedSearch: true, // Enables exact match using '=' - }); - - // Check if query length is above 40 characters have an exact match (likely a contract address) - const searchQuery = - debouncedSearchText.length > 40 - ? `="${debouncedSearchText}"` - : debouncedSearchText; - - const results = fuse.search(searchQuery).map((r) => r.item); - dispatch(setSearchTokenResult(results)); - } - - const fuse = new Fuse(tokensWithBalances, { - keys: ['name', 'symbol', 'contract'], // Fields to search in - threshold: 0.3, // Allow some fuzziness for queries that are not contract like - }); - - const results = fuse - .search(debouncedSearchText) - .map((token) => token.item); - - dispatch( - setSearchTokenResult( - swapChain.chainId === 0 - ? results - : results - .filter( - (tokens) => - chainNameToChainIdTokensData(tokens.blockchain) === - swapChain.chainId - ) - .map((tokens) => tokens) - ) - ); + if (debouncedSearchText.length > 40) { + // Exact match (likely a contract address) + const fuse = new Fuse(tokensWithBalances, { + keys: ['name', 'symbol', 'contract'], + threshold: 0.2, + minMatchCharLength: 3, + useExtendedSearch: true, + }); + const searchQuery = `="${debouncedSearchText}"`; + const results = fuse.search(searchQuery).map((r) => r.item); + const filtered = + swapChain.chainId === 0 + ? results + : results.filter( + (t) => + chainNameToChainIdTokensData(t.blockchain) === + swapChain.chainId + ); + dispatch(setSearchTokenResult(filtered)); + } else { + // Fuzzy search + const fuse = new Fuse(tokensWithBalances, { + keys: ['name', 'symbol', 'contract'], + threshold: 0.3, + }); + const results = fuse.search(debouncedSearchText).map((r) => r.item); + const filtered = + swapChain.chainId === 0 + ? results + : results.filter( + (t) => + chainNameToChainIdTokensData(t.blockchain) === + swapChain.chainId + ); + dispatch(setSearchTokenResult(filtered)); + }Also applies to: 154-176
src/components/BottomMenuModal/HistoryModal/TransactionsHistory.tsx (1)
86-91: Potential crash when transactions.outgoing is undefined
transactions?.outgoing.lengthwill throw iftransactionsexists butoutgoingis undefined/null. Use optional chaining onoutgoingtoo.Apply this diff:
- const allOutgoingTransactions = transactions?.outgoing.length + const allOutgoingTransactions = transactions?.outgoing?.length ? transactions.outgoing.map((transaction) => ({ ...transaction, type: 'outgoing', })) : [];src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (1)
141-149: Confirm presence API accepts missing address — otherwise gate the callpillarXApiPresence simply POSTs the provided payload as the request body (src/services/pillarXApiPresence.ts). Many callers pass address: accountAddress without guarding for undefined. Either confirm the backend accepts a missing/empty address or update the client to omit or gate the field.
Files to check / fix:
- src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx — handleClick() recordPresence calls (lines ~141–149 and ~160–169).
- src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx — useEffect recordPresence (around lines ~188–194).
- src/apps/the-exchange/components/SelectDropdown/SelectDropdown.tsx — recordPresence calls (~lines 78–95).
- src/components/AppsList.tsx — onClick recordPresence (~lines 81–86).
(There are additional occurrences across the codebase; search for recordPresence usages.)Suggested fixes (choose one depending on API contract):
- If API accepts missing address: avoid sending undefined by conditionally including the field:
recordPresence({
...(accountAddress && { address: accountAddress }),
action: 'app:theExchange:sourceTokenSelect',
value: { /* ... */ },
});- If API requires an address: gate the call:
if (!accountAddress) return;
recordPresence({ address: accountAddress, action: ..., value: {...} });Please confirm the expected server behaviour and apply the appropriate change across the listed locations (and any other unguarded recordPresence calls).
src/apps/leaderboard/components/LeaderboardTab/LeaderboardTab.tsx (1)
141-146: Fix potential runtime crash when indexing addresses with optional chaining
result.addresses?.[result.addresses.length - 1]can throw ifaddressesis undefined, sinceresult.addresses.lengthis still evaluated. Use a safe last-element access.Apply this diff to prevent crashes:
- <div - key={`${result.addresses?.[result.addresses.length - 1]}-${index}`} + <div + key={`${(result.addresses?.slice(-1)[0] ?? '')}-${index}`} className="grid desktop:grid-cols-[70%_15%_15%] tablet:grid-cols-[70%_15%_15%] mobile:grid-cols-[70%_30%] items-center desktop:py-5 tablet:py-5 mobile:py-[5px]" > ... - walletAddress={ - result.addresses?.[result.addresses.length - 1] || '' - } + walletAddress={result.addresses?.slice(-1)[0] || ''}Also applies to: 149-153
src/apps/token-atlas/components/TokensSlider/TokensSlider.tsx (2)
74-93: Debounce cleanup is misplaced; stale address can be recorded
- The cleanup returned from
handleHorizontalScrollis never used by the event system. Debounce cancellation should be in an effect cleanup.- Since the scroll listener is attached once and not updated, it captures the initial
accountAddressand may record presence with a stale or undefined address.Refactor to memoize the debounced function and the handler, and clean up properly:
-import _ from 'lodash'; -import { useEffect, useRef } from 'react'; +import _ from 'lodash'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; ... - const debouncedTokenTrendingScroll = _.debounce(() => { - recordPresence({ - address: accountAddress, - action: 'app:tokenAtlas:trendingScroll', - value: 'TRENDING_SCROLL', - }); - }, 2000); + const debouncedTokenTrendingScroll = useMemo( + () => + _.debounce(() => { + recordPresence({ + address: accountAddress, + action: 'app:tokenAtlas:trendingScroll', + value: 'TRENDING_SCROLL', + }); + }, 2000), + [recordPresence, accountAddress], + ); // Handle the scroll event - const handleHorizontalScroll = () => { - if (sliderRef.current) { - debouncedTokenTrendingScroll(); - } - - // Clean-up debounce on component unmount - return () => { - debouncedTokenTrendingScroll.cancel(); - }; - }; + const handleHorizontalScroll = useCallback(() => { + if (sliderRef.current) debouncedTokenTrendingScroll(); + }, [debouncedTokenTrendingScroll]);
95-108: Attach/detach scroll listener correctly and cancel debounce on unmountEnsure the listener uses the latest handler and the debounce timer is cancelled.
- useEffect(() => { - const sliderElement = sliderRef.current; - if (sliderElement) { - sliderElement.addEventListener('scroll', handleHorizontalScroll); - } - - return () => { - if (sliderElement) { - sliderElement.removeEventListener('scroll', handleHorizontalScroll); - } - }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sliderRef]); + useEffect(() => { + const sliderElement = sliderRef.current; + if (!sliderElement) return; + + sliderElement.addEventListener('scroll', handleHorizontalScroll); + return () => { + sliderElement.removeEventListener('scroll', handleHorizontalScroll); + debouncedTokenTrendingScroll.cancel(); + }; + }, [handleHorizontalScroll, debouncedTokenTrendingScroll]);src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx (2)
170-173: Fix styling typo: "white" → "text-white"The Tailwind class should be
text-white. As written,whitelikely has no effect.- <p className="text-xs white font-normal"> + <p className="text-xs text-white font-normal">
232-256: Guard explorer link by ensuring walletAddress existsIf
walletAddressis undefined, the explorer URL becomes invalid. Only render the button when an address is available.- {(displayStatus === 'Failed' || displayChainId === '137') && - displayChainId && ( + {(displayStatus === 'Failed' || displayChainId === '137') && + displayChainId && walletAddress && ( <button type="button" className="flex bg-purple_medium rounded-md justify-between px-4 py-2 self-center items-center gap-1" data-url={`${getBlockScan(Number(displayChainId), true)}${walletAddress}`} onClick={() => window.open( `${getBlockScan(Number(displayChainId), true)}${walletAddress}`, '_blank', 'noopener,noreferrer' ) } >src/apps/token-atlas/components/TokensSearchInput/TokensSearchInput.tsx (1)
77-105: Fix dependency array: body uses chainId but deps track chainNameThe effect filters results based on selectedChain.chainId but the dependency array tracks selectedChain.chainName. This can lead to stale filtering when chainId changes without a chainName change.
Apply this diff to correct the dependency:
- }, [searchData, debouncedSearchText, selectedChain.chainName]); + }, [searchData, debouncedSearchText, selectedChain.chainId]);src/apps/the-exchange/index.tsx (3)
152-176: After adding a chain (4902), attempt switch againMany wallets require a follow-up wallet_switchEthereumChain call after wallet_addEthereumChain. Implement a second switch to improve reliability.
Apply this diff:
- if ((switchError as { code?: number }).code === 4902) { - await providerWithRequest.request({ - method: 'wallet_addEthereumChain', - params: [ - { - chainId: `0x${chainId.toString(16)}`, - chainName: targetChain.name, - nativeCurrency: targetChain.nativeCurrency, - rpcUrls: targetChain.rpcUrls.default.http, - blockExplorerUrls: targetChain.blockExplorers - ?.default?.url - ? [targetChain.blockExplorers.default.url] - : undefined, - }, - ], - }); - } else { + if ((switchError as { code?: number }).code === 4902) { + await providerWithRequest.request({ + method: 'wallet_addEthereumChain', + params: [ + { + chainId: `0x${chainId.toString(16)}`, + chainName: targetChain.name, + nativeCurrency: targetChain.nativeCurrency, + rpcUrls: targetChain.rpcUrls.default.http, + blockExplorerUrls: targetChain.blockExplorers?.default?.url + ? [targetChain.blockExplorers.default.url] + : undefined, + }, + ], + }); + // Attempt switching again after adding + await providerWithRequest.request({ + method: 'wallet_switchEthereumChain', + params: [{ chainId: `0x${chainId.toString(16)}` }], + }); + } else { throw switchError; }
88-95: Guard against missing VITE_LIFI_API_KEY when initializing LiFi configcreateConfig is called with import.meta.env.VITE_LIFI_API_KEY in src/apps/the-exchange/index.tsx (around lines 88–95). If the env var is absent, abort initialization and log an error to avoid a silent misconfiguration.
- Files to change:
- src/apps/the-exchange/index.tsx — around the createConfig call (≈ lines 88–95)
Apply this diff:
- createConfig({ + if (!import.meta.env.VITE_LIFI_API_KEY) { + logExchangeEvent( + 'Missing VITE_LIFI_API_KEY', + 'error', + { walletAddress }, + { component: 'App', action: 'config_init_missing_api_key' } + ); + return; + } + createConfig({ integrator: 'PillarX',
88-96: Return a viem WalletClient from EVM.getWalletClient (don’t pass an EIP‑1193 provider directly)LiFi’s EVM provider expects a viem WalletClient; returning an EIP‑1193 provider (even cast as any) is brittle and may break on upgrades.
- File to change:
- src/apps/the-exchange/index.tsx — lines ~88–96: update getWalletClient to return a viem WalletClient.
Suggested replacement snippet:
import { createWalletClient, custom } from 'viem' import { mainnet } from 'viem/chains' // adjust to your chain createConfig({ integrator: 'PillarX', providers: [ EVM({ // return a viem WalletClient instead of an EIP-1193 provider getWalletClient: async () => createWalletClient({ chain: mainnet, // use your configured chain transport: custom(provider as any) }), /** * Chain switching functionalityAlternative (if you use Wagmi):
EVM({ getWalletClient: () => getWalletClient(wagmiConfig) })src/apps/the-exchange/hooks/useOffer.tsx (3)
520-526: Critical: string comparison for allowance check leads to incorrect approvalsformatUnits returns strings; comparing strings with >= is lexicographic and will yield incorrect results for many values. Compare BigInts directly in token units.
- const isEnoughAllowance = isAllowance - ? formatUnits(isAllowance, step.action.fromToken.decimals) >= - formatUnits( - BigInt(step.action.fromAmount), - step.action.fromToken.decimals - ) - : undefined; + const requiredAmount = BigInt(step.action.fromAmount); + const isEnoughAllowance = isAllowance + ? isAllowance >= requiredAmount + : undefined;
340-354: Critical: native balance check omits fee — risk of insufficient funds at executionFor native token swaps, totalNativeRequired excludes the feeAmount, so a wallet with exactly the swap amount passes validation but will fail when paying the fee.
- // Native: swap amount + fee - // Use fromAmountBigInt for total required - totalNativeRequired = fromAmountBigInt; + // Native: swap amount + fee (and deposit if wrapping) + totalNativeRequired = fromAmountBigInt + feeAmount; if (isWrapRequired) { - totalNativeRequired += fromAmountBigInt - feeAmount; // wrapping step uses swap amount + // wrapping step uses the swap amount (input minus fee) + totalNativeRequired += fromAmountBigInt - feeAmount; }
10-17: Validate fee receiver address (prevent misconfiguration or zero-address sends)Currently only presence is validated. Add a proper address check to prevent funds from being sent to an invalid or zero address.
import { createPublicClient, encodeFunctionData, erc20Abi, formatUnits, http, parseUnits, zeroAddress, + isAddress, } from 'viem';- // Validate fee receiver address - if (!feeReceiver) { - throw new Error('Fee receiver address is not configured'); - } + // Validate fee receiver address + if (!feeReceiver || !isAddress(feeReceiver) || isZeroAddress(feeReceiver)) { + throw new Error('Fee receiver address is invalid or not configured'); + }Also applies to: 308-314
♻️ Duplicate comments (2)
src/services/walletConnect.ts (1)
1254-1254: Duplicate dependency array commentSame issue as Line 1207 - the dependency array includes
kit.getSdkwhich may be unnecessary.src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (1)
324-470: Extract UserOp status monitoring logic into a reusable functionSimilar to SendModalTokensTabView, this component also has extensive UserOp polling logic that could be extracted for reusability and easier testing.
The UserOp status monitoring logic (lines 324-470) is nearly identical to the logic in SendModalTokensTabView. This should be extracted into a shared utility or hook to avoid duplication and ensure consistent behavior across components.
Consider creating a shared
useUserOpStatusMonitoringhook that both components can use.
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Just a few comments / questions / suggestions from me - please check the CodeRabbit suggestions also as some of them (i've commented / tagged you) are relevant. Overall though fantastic work and can't wait to get TxKit 2 in place!
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx
Outdated
Show resolved
Hide resolved
src/apps/token-atlas/components/SelectChainDropdown/SelectChainDropdown.tsx
Show resolved
Hide resolved
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/apps/pillarx-app/components/AnimatedTile/AnimatedTitle.tsx (1)
38-51: EnsureuseEffectcaptures late-arrivingaccountAddressand include all dependenciesThe current effect only reruns on
animationComplete, so ifaccountAddressbecomes available afterward,recordPresencenever fires. The ESLint disable also hides missing deps (accountAddress,data.id,data.layout,tileDisplay?.title, andrecordPresence). Refactor by early-returning when preconditions aren’t met, removing the lint suppression, and listing every referenced value in the dependency array.File: src/apps/pillarx-app/components/AnimatedTile/AnimatedTitle.tsx
Location: around theuseEffecton lines 36–50Suggested changes:
Extract
tileNameabove the effect:const tileName = tileDisplay?.title || '';Replace the effect with:
- useEffect(() => { - if (animationComplete && accountAddress) { - recordPresence({ - address: accountAddress, - action: 'app:feed:viewTile', - value: { - tileId: data.id, - tileName: tileDisplay?.title || '', - layout: data.layout, - }, - }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [animationComplete]); + useEffect(() => { + if (!animationComplete || !accountAddress) return; + + recordPresence({ + address: accountAddress, + action: 'app:feed:viewTile', + value: { + tileId: data.id, + tileName, + layout: data.layout, + }, + }); + }, [ + animationComplete, + accountAddress, + data.id, + data.layout, + tileName, + recordPresence, + ]);Optional: to guarantee “at most once per visible session,” add a
useRefflag reset wheninViewturns false or whendata.idchanges. Let me know if you’d like that snippet.src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (1)
207-213: Fix itemCount vs itemData length mismatch (can cause out-of-bounds renders)
itemsListNumber()returns the full portfolio length, butitemsList()may filter byswapChain.chainId. When filtered,itemCountcan exceedtokenList.length, risking undefined row data inreact-window.Apply this diff to keep lengths in sync:
- const itemsListNumber = () => { - if (isSwapOpen && !searchToken && walletPortfolio) { - return convertPortfolioAPIResponseToToken(walletPortfolio).length; - } - return searchTokenResult?.length || 0; - }; + const itemsListNumber = () => itemsList().length;Optionally, memoize
itemsList()to avoid recomputation:// outside diff context const items = useMemo(itemsList, [isSwapOpen, searchToken, walletPortfolio, swapChain.chainId, searchTokenResult]);Then use
items.lengthand passitemsintoitemData.src/utils/blockchain.ts (1)
74-76: Bug: address expression always resolves to AddressZeroUsing
ethers.constants.AddressZero || (chainId === 137 && WRAPPED_POL_TOKEN_ADDRESS)will always return AddressZero (non-empty string is truthy), so WRAPPED_POL is never selected on Polygon. Use a conditional expression.Apply this diff:
- address: - ethers.constants.AddressZero || - (chainId === 137 && WRAPPED_POL_TOKEN_ADDRESS), + address: chainId === 137 ? WRAPPED_POL_TOKEN_ADDRESS : ethers.constants.AddressZero,
♻️ Duplicate comments (11)
src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx (1)
97-108: Dispatching actions via the store in beforeEach: resolved and correctThis addresses the earlier issue (actions were called instead of dispatched). Seeding the state via store.dispatch before render ensures selectors see the expected values.
Optional: consider extracting these dispatches into a helper (e.g., seedExchangeState) in test-utils to reduce duplication across tests.
src/providers/EtherspotTransactionKitProvider.tsx (3)
1-1: Remove the ESLint disable — the context value is already memoizedYou’re properly memoizing the context value; the rule disable is no longer needed.
Apply this diff:
-/* eslint-disable react/jsx-no-constructed-context-values */
43-56: You’re still re-instantiating the kit on every chain/config change — keep a single instance and update in placeBecause
kitConfigdepends onactiveChainId,useMemocreates a newEtherspotTransactionKitwhenever the chain changes. This undermines continuity, loses pending state, and makes the update effect redundant. Maintain a single instance in a ref and update its config in the effect.Apply this diff:
- const kitConfig = useMemo( - () => ({ - ...config, - chainId: activeChainId ?? config.chainId, - }), - [config, activeChainId] - ); - - const kit = useMemo(() => { - const newKit = new EtherspotTransactionKit(kitConfig); - kitRef.current = newKit; - return newKit; - }, [kitConfig]); + // Create a single kit instance and keep it stable across renders. + if (!kitRef.current) { + kitRef.current = new EtherspotTransactionKit(config); + } + const kit = kitRef.current;Follow-up: With a stable instance, the chain switch logic in the effect below becomes the sole pathway, preserving state and avoiding connection resets.
93-102: Context payload includes an internal setter not in the type (and not needed)
setWalletAddressis exposed but not part ofEtherspotTransactionKitContextType, and prior analysis showed no external usage. Remove it for a tighter, safer API surface.Apply this diff:
const contextData = useMemo( () => ({ walletAddress, - setWalletAddress, kit, activeChainId, setActiveChainId, }), [walletAddress, kit, activeChainId] );src/components/BottomMenuModal/AccountModal.tsx (1)
58-60: Token-specific image error handling: type-safety and minor perf improvementGood fix addressing the previous “shared hideImage” issue. Two small tweaks:
- Use a Set for safety (asset IDs are commonly strings across APIs).
- Update the Set without Array round-trip.
Apply:
- const [failedImages, setFailedImages] = React.useState<Set<number>>( - new Set() - ); + const [failedImages, setFailedImages] = React.useState<Set<string>>(new Set());- {!failedImages.has(asset.asset.id) && asset.asset.logo ? ( + {!failedImages.has(String(asset.asset.id)) && asset.asset.logo ? ( <img src={asset.asset.logo} alt={asset.asset.name} - onError={() => - setFailedImages( - (prev) => - new Set(Array.from(prev).concat(asset.asset.id)) - ) - } + onError={() => + setFailedImages((prev) => { + const next = new Set(prev); + next.add(String(asset.asset.id)); + return next; + }) + } />Optional: create a local const key = String(asset.asset.id) inside the map and reuse it for readability.
Also applies to: 254-263
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (2)
137-154: Good fix: actions are now dispatched to the storeSwitching from direct action creator calls to
store.dispatch(...)in beforeEach correctly updates Redux state for the tests. This addresses the earlier issue where state was never updated.
86-91: Type the mockChains object to match real ChainTypeAnnotate
mockChainswith the actualChainTypeto keep tests aligned with production types and catch shape drift at compile time.Apply this diff:
-const mockChains = { +const mockChains: Record<string, ChainType> = { ethereum: { chainId: 1, chainName: 'Ethereum' }, polygon: { chainId: 137, chainName: 'Polygon' }, arbitrum: { chainId: 42161, chainName: 'Arbitrum' }, };Add the type-only import near the other imports:
// add alongside other imports import type { ChainType } from '../../../utils/types';src/utils/blockchain.ts (1)
357-415: Unify amount/value types in buildTransactionData (precision + type safety)Currently accepts
amount: numberand returnsvalue: bigint(native) vs'0'(string) for ERC20. This mixes types and invites precision issues with JS numbers.Apply this diff to accept
string | bigintand always returnvalue: bigint(0nfor ERC20), with stronger address typing:-export const buildTransactionData = ({ +export const buildTransactionData = ({ tokenAddress, recipient, amount, decimals, }: { - tokenAddress: string; - recipient: string; - amount: number; - decimals: number; -}) => { + tokenAddress: string; + recipient: `0x${string}`; + /** + * amount: + * - string: human-readable units (e.g. '1.23') + * - bigint: base units (already parsed to token decimals) + */ + amount: string | bigint; + decimals: number; +}): { to: `0x${string}`; value: bigint; data: `0x${string}` } => { // Validate recipient address - if (!recipient || !isValidEthereumAddress(recipient)) { + if (!recipient || !isValidEthereumAddress(recipient)) { throw new Error('Invalid recipient address'); } // Validate amount - if (amount <= 0 || !Number.isFinite(amount) || Number.isNaN(amount)) { + if ( + (typeof amount === 'string' && (amount.trim() === '' || Number(amount) <= 0 || Number.isNaN(Number(amount)))) || + (typeof amount === 'bigint' && amount <= 0n) + ) { throw new Error('Invalid amount: must be a positive finite number'); } // Validate decimals if (decimals < 0 || decimals > 18 || !Number.isInteger(decimals)) { throw new Error('Invalid decimals: must be an integer between 0 and 18'); } // Validate token address (for ERC20 tokens) if (!isNativeToken(tokenAddress) && !isValidEthereumAddress(tokenAddress)) { throw new Error('Invalid token address'); } try { + const parsedAmount = + typeof amount === 'bigint' ? amount : parseUnits(amount, decimals); + if (isNativeToken(tokenAddress)) { // Native token transfer return { - to: recipient, - value: parseUnits(amount.toString(), decimals), - data: '0x', + to: recipient, + value: parsedAmount, + data: '0x', }; } // ERC20 transfer return { - to: tokenAddress, - value: '0', + to: tokenAddress as `0x${string}`, + value: 0n, data: encodeFunctionData({ abi: erc20Abi, functionName: 'transfer', - args: [ - recipient as `0x${string}`, - parseUnits(amount.toString(), decimals), - ], + args: [recipient, parsedAmount], }), }; } catch (error) { throw new Error( `Failed to build transaction data: ${error instanceof Error ? error.message : 'Unknown error'}` ); } };Follow-up: update call sites to pass amount as a string (recommended) or bigint in base units.
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (2)
567-594: Differentiate not-found vs unexpected errors when removing a batch, and don’t silently swallow unexpected ones.This improves ops signal-to-noise and user clarity.
- } catch (e) { - console.error('Failed to remove batch:', e); + } catch (e) { + const isNotFoundError = + e instanceof Error && e.message.toLowerCase().includes('not found'); + if (!isNotFoundError) { + console.error('Failed to remove batch:', e); + } const batchTransactions = batches[batchName]; const chainId = batchTransactions?.[0]?.chainId; if (typeof chainId === 'number') { setErrorMessage((prev) => ({ ...prev, - [chainId]: t`error.failedToRemoveBatch`, + [chainId]: isNotFoundError + ? t('error.batchAlreadyRemoved') + : t('error.failedToRemoveBatch'), })); } - Sentry.captureMessage('Failed to remove batch', { - level: 'warning', + Sentry.captureMessage('Failed to remove batch', { + level: isNotFoundError ? 'info' : 'warning', tags: { component: 'send_flow', action: 'remove_batch_failed', removeId, }, contexts: { remove_batch_failed: { removeId, batchName, error: e instanceof Error ? e.message : String(e), }, }, }); }
507-528: Use the original sendId in the catch block (avoid generating a new one).Regenerating sendId in the error path breaks traceability in Sentry.
- Sentry.captureException(error, { + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'batch_send_error', - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + sendId, }, contexts: { batch_send_error: { - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + sendId, chainId, batchName, error: error instanceof Error ? error.message : String(error), }, }, });src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
2049-2066: Validate payloadTx “to” before adding to batch (batching payload single-transaction).This path still allows empty/invalid to, contrary to the guarded send flow.
} else if (payload && 'transaction' in payload) { // Single payload transaction batching const payloadTxAddToBatch = payload.transaction as ITransaction; + if (!payloadTxAddToBatch?.to || !isAddress(payloadTxAddToBatch.to)) { + setErrorMessage(t`error.invalidRecipientAddress`); + return; + } txData = { to: payloadTxAddToBatch?.to || '', value: payloadTxAddToBatch?.value !== undefined ? String(payloadTxAddToBatch.value) : '0', data: payloadTxAddToBatch?.data || undefined,
🧹 Nitpick comments (53)
src/hooks/__tests__/useAccountTransactionHistory.test.tsx (3)
1-1: Importactif you plan to assert state updates via settersIf you extend the test to exercise setters (suggestion below), you’ll need
act.Apply this diff:
-import { renderHook } from '@testing-library/react'; +import { renderHook, act } from '@testing-library/react';
29-34: Deduplicate the inline provider wrapper (DRY)You define an identical
wrapperin two tests. Extract a shared Providers wrapper to reduce duplication and improve readability.Example (outside the tests):
const Providers = ({ children }: React.PropsWithChildren) => ( <AccountTransactionHistoryProvider>{children}</AccountTransactionHistoryProvider> );Then, in both tests:
renderHook(() => useAccountTransactionHistory(), { wrapper: Providers });
29-48: Rename test to reflect intent and add a minimal behavior check for a setterThe test name says “returns user-op data” but only validates initial state. Renaming improves clarity. Also, lightly exercising one setter (e.g.,
setTransactionHash) guards against regressions and proves the provider’s state updates work.Apply this diff:
-it('returns user-op data when parent provider exists', () => { +it('exposes initial state and setters when wrapped with provider', () => { @@ - expect(result.current).toBeDefined(); - expect(result.current.userOpStatus).toBeUndefined(); // Initial state - expect(result.current.transactionHash).toBeUndefined(); // Initial state - expect(result.current.latestUserOpInfo).toBeUndefined(); // Initial state - expect(result.current.latestUserOpChainId).toBeUndefined(); // Initial state - expect(typeof result.current.setUserOpStatus).toBe('function'); - expect(typeof result.current.setTransactionHash).toBe('function'); - expect(typeof result.current.setLatestUserOpInfo).toBe('function'); - expect(typeof result.current.setLatestUserOpChainId).toBe('function'); + expect(result.current).toBeDefined(); + expect(result.current.userOpStatus).toBeUndefined(); // Initial state + expect(result.current.transactionHash).toBeUndefined(); // Initial state + expect(result.current.latestUserOpInfo).toBeUndefined(); // Initial state + expect(result.current.latestUserOpChainId).toBeUndefined(); // Initial state + expect(typeof result.current.setUserOpStatus).toBe('function'); + expect(typeof result.current.setTransactionHash).toBe('function'); + expect(typeof result.current.setLatestUserOpInfo).toBe('function'); + expect(typeof result.current.setLatestUserOpChainId).toBe('function'); + + // Minimal behavior check: update a field and assert it changes + act(() => { + result.current.setTransactionHash('0xdeadbeef'); + }); + expect(result.current.transactionHash).toBe('0xdeadbeef');Note: This relies on the
actimport suggested above.src/apps/pillarx-app/components/AnimatedTile/AnimatedTitle.tsx (2)
31-33: Minor cleanup and prep for stable deps: avoiddata || {}and computetileNameonce.
datais typed (non-optional) and you can de-structure without the fallback. Also computetileNameonce to use in the effect deps for stability and readability.Apply this diff:
- const { meta } = data || {}; - const tileDisplay = meta?.display; + const { meta } = data; + const tileDisplay = meta?.display; + const tileName = tileDisplay?.title ?? '';
11-21: Naming inconsistency: file is “AnimatedTitle.tsx” but component/export is “AnimatedTile”.This is a footgun for discoverability and future imports. Align the file and component names.
Two options (pick one):
- Rename the file to
AnimatedTile.tsx(preferred, matches folder name and component).- Or rename the component to
AnimatedTitleand export that:-const AnimatedTile = ({ +const AnimatedTitle = ({ children, isDataLoading, data, accountAddress, }: { children: ReactNode; isDataLoading: boolean; data: Projection; accountAddress?: string; }) => { ... -export default AnimatedTile; +export default AnimatedTitle;If you opt to rename the component instead of the file, please verify imports. I can provide a script to locate and update references across the repo.
Also applies to: 73-73
src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx (6)
1-2: Prefer userEvent over fireEvent for realistic interactionsuserEvent better simulates user typing and avoids certain sync pitfalls. Replace fireEvent with userEvent and adjust the assertion accordingly.
Apply this diff to update imports:
-import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';
130-133: Remove the “test store issues” comment and assert the initial valueGiven the state is now seeded correctly, assert the initial value instead of leaving a caveat comment.
Apply this diff:
- // The input should be present but may not have the expected initial value due to test store issues - const input = screen.getByTestId('enter-amount-input'); - expect(input).toBeInTheDocument(); + const input = screen.getByTestId('enter-amount-input'); + expect(input).toBeInTheDocument(); + // Initial value seeded in beforeEach + // If the input is type="number", switch to: expect(input).toHaveValue(0.1); + expect(input).toHaveDisplayValue('0.1');
135-135: Use userEvent for typing into the inputuserEvent more closely matches how users interact with inputs and can avoid sync issues.
Apply this diff (paired with the import change suggested on Lines 1-2):
- fireEvent.change(input, { target: { value: '0.5' } }); + await userEvent.clear(input); + await userEvent.type(input, '0.5');
137-140: Assert on the displayed value instead of numeric value to avoid type mismatchesDepending on the input’s type, toHaveValue(0.5) can be brittle. toHaveDisplayValue('0.5') is robust for text/number inputs and matches what users see.
Apply this diff:
- await waitFor(() => { - expect(input).toHaveValue(0.5); - }); + await waitFor(() => expect(input).toHaveDisplayValue('0.5'));
142-145: Redundant mock clearingYou call vi.clearAllMocks() in both beforeEach and afterEach. Keeping it in one place is sufficient; afterEach is usually preferred.
Update outside this hunk (Line 96):
- Remove the vi.clearAllMocks() call in beforeEach and keep the afterEach cleanup.
112-118: Optional: Snapshot via RTL instead of react-test-rendererTo reduce test surface area and stick to one renderer, consider RTL snapshots using asFragment and drop react-test-renderer.
Apply this diff:
- it('renders correctly and matches snapshot', () => { - const tree = renderer - .create( - <ExchangeTestWrapper> - <EnterAmount type={CardPosition.SWAP} /> - </ExchangeTestWrapper> - ) - .toJSON(); - expect(tree).toMatchSnapshot(); - }); + it('renders correctly and matches snapshot', () => { + const { asFragment } = render( + <ExchangeTestWrapper> + <EnterAmount type={CardPosition.SWAP} /> + </ExchangeTestWrapper> + ); + expect(asFragment()).toMatchSnapshot(); + });And remove the unused import:
-import renderer from 'react-test-renderer';src/providers/EtherspotTransactionKitProvider.tsx (2)
75-81: Harden wallet address refresh on chain switch (handle errors)Protect against thrown/rejected calls from the kit to avoid unhandled rejections during chain changes.
Apply this diff:
- const updateWalletAddress = async () => { - const address = await kitRef.current?.getWalletAddress(); - if (address) setWalletAddress(address); - }; + const updateWalletAddress = async () => { + try { + const address = await kitRef.current?.getWalletAddress(); + if (address) setWalletAddress(address); + } catch (err) { + // eslint-disable-next-line no-console + console.error('Failed to get wallet address after chain switch', err); + } + };
85-91: Add error handling during initial wallet address fetchSame reasoning as above; avoid unhandled rejections on mount.
Apply this diff:
useEffect(() => { const init = async () => { - const address = await kit.getWalletAddress(); - if (address) setWalletAddress(address); + try { + const address = await kit.getWalletAddress(); + if (address) setWalletAddress(address); + } catch (err) { + // eslint-disable-next-line no-console + console.error('Failed to get wallet address on init', err); + } }; init(); }, [kit]);src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx (3)
109-118: Assert the chain actually changes after invoking setActiveChainIdYou’re clicking the button but not asserting the state change. Strengthen the test by verifying the chain id updates to 5.
Apply this diff:
// Click the button to change chain act(() => { screen.getByTestId('change-chain').click(); }); - // The setActiveChainId function should be callable - expect(screen.getByTestId('change-chain')).toBeInTheDocument(); + // The setActiveChainId function should be callable and update the context value + expect(screen.getByTestId('change-chain')).toBeInTheDocument(); + expect(screen.getByTestId('current-chain')).toHaveTextContent('5');
33-73: Consider asserting values (not just presence) to catch regressionsOptional: strengthen expectations by asserting specific content, e.g. active chain “1” initially, or that a wallet address eventually appears (using findBy*/waitFor) if you mock the kit.
5-10: Confirm whether you intend to use the real provider or the mock in testsRight now the test imports the real provider. If isolation is preferred, explicitly mock the provider so the test doesn’t instantiate the real kit.
Example approach (one option):
// At the very top of the file (before importing the provider) import { vi } from 'vitest'; vi.mock('../EtherspotTransactionKitProvider', async () => { // Re-export the mock provider from __mocks__ return await import('../../../__mocks__/EtherspotTransactionKitProvider'); });src/providers/__tests__/AccountTransactionHistoryProvider.test.tsx (5)
24-35: Use nullish coalescing (??) instead of || to avoid misclassifying falsy valuesUsing || will render "undefined" when the value is a valid falsy (e.g., empty string '' or 0). Nullish coalescing (??) only falls back for null/undefined and is safer for status and chainId.
Apply this diff:
- <div data-testid="user-op-status"> - {context.data.userOpStatus || 'undefined'} - </div> + <div data-testid="user-op-status"> + {context.data.userOpStatus ?? 'undefined'} + </div> <div data-testid="transaction-hash"> - {context.data.transactionHash || 'undefined'} + {context.data.transactionHash ?? 'undefined'} </div> <div data-testid="latest-user-op-info"> - {context.data.latestUserOpInfo || 'undefined'} + {context.data.latestUserOpInfo ?? 'undefined'} </div> <div data-testid="latest-user-op-chain-id"> - {context.data.latestUserOpChainId || 'undefined'} + {context.data.latestUserOpChainId ?? 'undefined'} </div>
1-7: Prefer userEvent over element.click() and drop manual act wrappersTesting Library recommends userEvent for realistic interactions (async, bubbles, defaultPrevented, etc.). It also eliminates the need for manual act() around clicks.
- Import userEvent and remove act from imports.
- Replace element.click() + act with await userEvent.click(...).
- Keep waitFor where appropriate.
Apply these diffs (pattern; replicate for other click sites):
-import { - act, - render, - renderHook, - screen, - waitFor, -} from '@testing-library/react'; +import { render, renderHook, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';- // Set to Sending - await act(async () => { - screen.getByTestId('set-sending').click(); - }); + // Set to Sending + await userEvent.click(screen.getByTestId('set-sending'));- // Set transaction hash - await act(async () => { - screen.getByTestId('set-transaction-hash').click(); - }); + // Set transaction hash + await userEvent.click(screen.getByTestId('set-transaction-hash'));And similarly for:
- set-sent
- set-confirmed
- set-failed
- set-latest-user-op-info
- set-latest-user-op-chain-id
Also applies to: 133-135, 142-144, 151-153, 162-164, 181-182, 201-202, 221-222, 254-255, 262-263, 286-287, 290-291, 294-295, 298-299
20-21: Fail fast on missing context to avoid false-positive passesReturning a placeholder UI hides misconfigurations. Throwing makes failures obvious if the provider isn’t mounted.
- if (!context) return <div>No context</div>; + if (!context) { + throw new Error('AccountTransactionHistoryContext is null'); + }
126-169: De-duplicate status transition checks with a table-driven loopThe repeated “click + assert” blocks can be compacted, improving readability and reducing maintenance.
Example refactor within this test:
- // Set to Sending - await act(async () => { - screen.getByTestId('set-sending').click(); - }); - await waitFor(() => { - expect(screen.getByTestId('user-op-status')).toHaveTextContent('Sending'); - }); - // Set to Sent - await act(async () => { - screen.getByTestId('set-sent').click(); - }); - await waitFor(() => { - expect(screen.getByTestId('user-op-status')).toHaveTextContent('Sent'); - }); - // Set to Confirmed - await act(async () => { - screen.getByTestId('set-confirmed').click(); - }); - await waitFor(() => { - expect(screen.getByTestId('user-op-status')).toHaveTextContent( - 'Confirmed' - ); - }); - // Set to Failed - await act(async () => { - screen.getByTestId('set-failed').click(); - }); - await waitFor(() => { - expect(screen.getByTestId('user-op-status')).toHaveTextContent('Failed'); - }); + const steps = [ + { id: 'set-sending', expected: 'Sending' }, + { id: 'set-sent', expected: 'Sent' }, + { id: 'set-confirmed', expected: 'Confirmed' }, + { id: 'set-failed', expected: 'Failed' }, + ]; + for (const { id, expected } of steps) { + await userEvent.click(screen.getByTestId(id)); + await waitFor(() => + expect(screen.getByTestId('user-op-status')).toHaveTextContent(expected) + ); + }
231-247: Tighten the assertion that context is non-nullYou already assert defined; adding an explicit non-null check clarifies intent and can allow removing optional chaining if desired.
- expect(result.current).toBeDefined(); + expect(result.current).toBeDefined(); + expect(result.current).not.toBeNull();Optionally, assign a local and drop optional chaining for stronger type guarantees:
const ctx = result.current!; expect(ctx.data).toBeDefined(); expect(ctx.data.userOpStatus).toBeUndefined(); // ...src/apps/the-exchange/components/SelectDropdown/SelectDropdown.tsx (2)
105-109: Avoid "undefined" leaking into classNameWhen
classNameis omitted, template interpolation currently injects the literal"undefined"into the class list.Apply this diff:
- className={`${className} h-[34px] z-20`} + className={`h-[34px] z-20 ${className ?? ''}`}
86-86: Preferelse ifto enforce mutual exclusivityIf both
isSwapOpenandisReceiveOpenare ever true, both branches will run. The UI likely intends only one selection at a time.Apply this diff:
- if (isReceiveOpen) { + else if (isReceiveOpen) {src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (3)
221-226: Remove redundant.map((tokens) => tokens)No-op map after filter creates unnecessary work and a new array reference.
Apply this diff:
- : tokensWithBalances - .filter( - (tokens) => - chainNameToChainIdTokensData(tokens.blockchain) === - swapChain.chainId - ) - .map((tokens) => tokens); + : tokensWithBalances.filter( + (tokens) => + chainNameToChainIdTokensData(tokens.blockchain) === + swapChain.chainId + );
269-271: Avoid"false"class leakage in template literalUsing
${isChainSelectionOpen && 'w-full'}yields the string"false"when the condition is falsy.Apply this diff:
- className={`${isChainSelectionOpen && 'w-full'}`} + className={isChainSelectionOpen ? 'w-full' : undefined}
185-201: UsetoLowerCase()for deterministic address comparisons
toLocaleLowerCase()can be locale-dependent. For addresses/contracts,toLowerCase()is safer and faster.Apply this diff:
- searchTokenResult[0].contract.toLocaleLowerCase() === - searchToken.toLocaleLowerCase() && + searchTokenResult[0].contract.toLowerCase() === + searchToken.toLowerCase() && @@ - searchTokenResult[0].contract.toLocaleLowerCase() !== - receiveToken.contract.toLocaleLowerCase() + searchTokenResult[0].contract.toLowerCase() !== + receiveToken.contract.toLowerCase() @@ - searchTokenResult[0].contract.toLocaleLowerCase() === - searchToken.toLocaleLowerCase() && + searchTokenResult[0].contract.toLowerCase() === + searchToken.toLowerCase() && @@ - searchTokenResult[0].contract.toLocaleLowerCase() !== - swapToken.contract.toLocaleLowerCase() + searchTokenResult[0].contract.toLowerCase() !== + swapToken.contract.toLowerCase()src/apps/pillarx-app/index.tsx (1)
104-112: Good: presence logging is now gated by walletAddress; consider moving to a dedicated effect and add error handlingCurrent call sits in an effect with exhaustive-deps disabled (Line 115). To avoid suppressed deps and ensure correct coupling to page changes, move presence reporting into its own effect with explicit dependencies and minimal duplication protection. Also consider handling promise rejection.
Apply this diff to remove the inline call from this effect:
- if (walletAddress) { - recordPresence({ - address: walletAddress, - action: 'app:feed:navigate', - value: { - pageNumber: page, - }, - }); - }Then add the following effect elsewhere in this component (e.g., below the existing effects):
// Ensures presence is recorded once per successful page load const lastPresencePageRef = useRef<number | null>(null); useEffect(() => { if (!walletAddress || !isHomeFeedSuccess) return; // Avoid duplicate presence logging for the same page if (lastPresencePageRef.current === page) return; lastPresencePageRef.current = page; // Fire-and-forget with basic error handling const p = recordPresence({ address: walletAddress, action: 'app:feed:navigate', value: { pageNumber: page }, }); // If unwrap is available (RTK Query), you can use it; otherwise swallow errors // @ts-expect-error unwrap might not exist depending on setup void (p.unwrap?.() ?? p).catch(() => { /* noop */ }); }, [walletAddress, isHomeFeedSuccess, page, recordPresence]);src/components/BottomMenuModal/AccountModal.tsx (2)
286-297: Accessibility: use a button with proper ARIA for the toggleThe toggle is a styled span. Use a button element with aria-expanded and aria-controls to improve accessibility and link it to the per-token panel.
Apply within usage:
- <ToggleButton + <ToggleButton + type="button" $expanded={expanded[`${symbol}-${asset.asset.id}`]} + aria-expanded={Boolean(expanded[`${symbol}-${asset.asset.id}`])} + aria-controls={`token-chains-account-modal-${symbol}-${asset.asset.id}`} onClick={() => setExpanded((prev) => ({ ...prev, [`${symbol}-${asset.asset.id}`]: !prev[`${symbol}-${asset.asset.id}`], })) } > <ArrowRightIcon size={15} /> </ToggleButton>And update the styled component definition:
-const ToggleButton = styled.span<{ $expanded: boolean }>` +const ToggleButton = styled.button<{ $expanded: boolean }>` transition: transform 0.2s ease-in-out; cursor: pointer; transform: ${({ $expanded }) => ($expanded ? 'rotate(-90deg)' : 'rotate(0)')}; padding: 3px; margin-left: 4px; + background: transparent; + border: 0;
217-245: Loading/empty states are handled; consider error state for failed fetchesThe skeletons and empty-state alert are cleanly gated by tokensLoading. If the query errors out (network/API), users will see a perpetual skeleton since success won’t flip true. Consider surfacing a specific error state via isError from useGetWalletPortfolioQuery.
Example:
// destructure from the query hook const { data: walletPortfolioData, isLoading: isWalletPortfolioDataLoading, isFetching: isWalletPortfolioDataFetching, isSuccess: isWalletPortfolioDataSuccess, isError: isWalletPortfolioDataError, } = useGetWalletPortfolioQuery(...); // in render (before or alongside the empty state) {isWalletPortfolioDataError && <Alert>{t`error.failedToLoad`}</Alert>}src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (8)
197-211: Avoid brittle class-based selector in text assertionUsing
{ selector: 'p.text-base' }couples the test to Tailwind classnames. Prefer matching the exact text within the scoped card.Apply this diff:
- expect(within(tokenCards[0]).getByText('Ether')).toBeInTheDocument(); - // This uses a more specific selector for POL to avoid ambiguity - expect( - within(tokenCards[1]).getByText('POL', { selector: 'p.text-base' }) - ).toBeInTheDocument(); + expect(within(tokenCards[0]).getByText(/^Ether$/)).toBeInTheDocument(); + // Avoid class-based selectors; match exact text within the card + expect(within(tokenCards[1]).getByText(/^POL$/)).toBeInTheDocument();
227-239: Brittle DOM structure assertion (Tailwind classes)Asserting via
closest('div.flex.w-full.justify-center')is fragile and will break on classname/order changes. Assert structure via test IDs instead.Apply this diff:
- // Main container should exist - const mainContainer = screen - .getByTestId('swap-receive-cards') - .closest('div.flex.w-full.justify-center'); - expect(mainContainer).toBeInTheDocument(); + // Main container should exist and contain two token cards + const mainContainer = screen.getByTestId('swap-receive-cards'); + expect( + within(mainContainer).getAllByTestId('select-token-card') + ).toHaveLength(2);
269-279: USD price assertion may be locale/format sensitiveHard-coding
"$0.00"is brittle if currency formatting, locale, or precision changes. Prefer asserting via a test id/role if exposed, or a more tolerant pattern.Options:
- If the component exposes a test id (e.g.,
data-testid="usd-price"), assert via that andtoHaveTextContent.- Otherwise, loosen the pattern (e.g.,
/^\$?\s?0(\.00)?$/) or assert the presence of an element that derives from 0 input rather than exact string.
281-293: Alt-text matching can be i18n-sensitive; make it resilientIf labels are localized, strict
'Send'/'Receive'may fail. Use case-insensitive regex or test IDs if available.Apply this diff:
- const sendArrow = screen.getByAltText('Send'); - const receiveArrow = screen.getByAltText('Receive'); + const sendArrow = screen.getByAltText(/send/i); + const receiveArrow = screen.getByAltText(/receive/i);
156-167: Snapshot breadth: consider narrowing scopeFull-tree snapshots through multiple providers can be noisy and brittle. Consider snapshotting a smaller, stable subtree (or use
toMatchInlineSnapshot) to reduce churn from unrelated provider/theme/router changes.
306-319: Use parameterized tests for state matrixThe forEach-with-render pattern works, but
it.eachortest.eachmakes failures easier to pinpoint and isolates each case as an individual test.Example:
it.each([ { swapToken: mockTokenAssets[0], receiveToken: mockTokenAssets[1] }, { swapToken: undefined, receiveToken: mockTokenAssets[1] }, { swapToken: mockTokenAssets[0], receiveToken: undefined }, { swapToken: undefined, receiveToken: undefined }, ])('renders with swap=%p receive=%p', ({ swapToken, receiveToken }) => { store.dispatch(setSwapToken(swapToken)); store.dispatch(setReceiveToken(receiveToken)); render( <ExchangeTestWrapper> <CardsSwap /> </ExchangeTestWrapper> ); expect(screen.getByTestId('swap-receive-cards')).toBeInTheDocument(); });
101-128: Centralize repeated token/chain mapping mocksThese chain mapping mocks are useful, but similar logic may be needed across multiple Exchange tests. Consider moving them into a shared test util (e.g., in
src/test-utils/mocks/tokensData.ts) and reusing to avoid drift between tests.
344-346: Redundant mock clearing
vi.clearAllMocks()is already called inbeforeEach. TheafterEachcall is redundant.Apply this diff to remove the duplicate cleanup:
- afterEach(() => { - vi.clearAllMocks(); - }); + // afterEach cleanup not required; mocks are cleared in beforeEach.src/apps/token-atlas/components/SelectChainDropdown/SelectChainDropdown.tsx (2)
43-43: Good: single source of wallet address via kitDestructuring walletAddress and aliasing to accountAddress is fine. Keep this naming consistent across Token Atlas to avoid confusion between account/wallet address terms.
66-71: Nit: remove redundant Number()option is already a number. Drop Number(...) to avoid needless work and keep types tight.
Apply this diff:
- dispatch( - setSelectedChain({ - chainId: Number(option), - chainName: option === 0 ? 'all' : getChainName(option), - }) - ); + dispatch( + setSelectedChain({ + chainId: option, + chainName: option === 0 ? 'all' : getChainName(option), + }) + );src/utils/blockchain.ts (1)
218-237: Nit: use HTTPS for Arbiscan URLMinor consistency/security tweak.
Apply this diff:
- case 42161: - return `http://arbiscan.io/${isAddress ? 'address' : 'tx'}/`; + case 42161: + return `https://arbiscan.io/${isAddress ? 'address' : 'tx'}/`;src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
262-273: Optional: drop ethers.BigNumber -> use bigint throughoutIf buildTransactionData returns
valueas bigint (see suggested refactor), this conversion block can be simplified to a single line with a nullish coalescing to 0n.Apply after unifying types:
- let bigIntValue: bigint; - if (typeof value === 'bigint') { - bigIntValue = value; - } else if (value) { - bigIntValue = BigNumber.from(value).toBigInt(); - } else { - bigIntValue = BigInt(0); - } + const bigIntValue: bigint = (value as bigint) ?? 0n;src/providers/GlobalTransactionsBatchProvider.tsx (1)
59-86: Avoid redundant state updates for batchCountGuard setting batchCount to reduce unnecessary renders on the 1s interval (React would bail on same value, but this avoids scheduling work).
Apply this diff:
- setBatchCount(Object.keys(batches).length); + const nextCount = Object.keys(batches).length; + setBatchCount((prev) => (prev !== nextCount ? nextCount : prev));src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (6)
90-104: Consolidate duplicate in-flight send guard (merge logging into the first guard and remove the second).The isSending check is performed twice. Keep the first check (with UI feedback) and move the Sentry logging there; remove the second block to avoid duplication and drift.
Apply these diffs:
- Add Sentry logging inside the first guard:
if (isSending[chainId]) { const chainName = getChainName(chainId); setErrorMessage((prev) => ({ ...prev, [chainId]: t('error.batchAlreadyInProgressWithChain', { chainName }), })); transactionDebugLog( 'Batch send blocked - another batch already in progress on this chain:', { chainId, batchName, isSending: isSending[chainId] } ); + + Sentry.captureMessage('Batch send disabled - another batch in progress', { + level: 'warning', + tags: { + component: 'send_flow', + action: 'batch_send_disabled', + sendId, + }, + contexts: { + batch_send_disabled: { + sendId, + chainId, + batchName, + isSending: isSending[chainId], + }, + }, + }); return; }
- Remove the second guard entirely:
- if (isSending[chainId]) { - transactionDebugLog( - 'Another batch is being sent, cannot process the sending of this batch:', - batchName - ); - - Sentry.captureMessage('Batch send disabled - another batch in progress', { - level: 'warning', - tags: { - component: 'send_flow', - action: 'batch_send_disabled', - sendId, - }, - contexts: { - batch_send_disabled: { - sendId, - chainId, - batchName, - isSending: isSending[chainId], - }, - }, - }); - - return; - } + // in-flight guard handled aboveAlso applies to: 119-143
497-503: Use correct decimals for estimatedCost in success context.Formatting the estimated cost with a hard-coded 18 can misrepresent non-18-decimal native assets. Use getNativeAssetForChainId(chainId).decimals for consistency with the estimation path above.
- estimatedCost: estimatedCostBN - ? ethers.utils.formatUnits(estimatedCostBN, 18) - : null, + estimatedCost: estimatedCostBN + ? ethers.utils.formatUnits( + estimatedCostBN, + getNativeAssetForChainId(chainId).decimals + ) + : null,
337-483: Add interval cleanup to prevent memory leaks during UserOp polling.The setInterval is cleared on several branches, but there’s no unmount cleanup; if the component unmounts mid-poll, the interval may continue.
Minimal approach: store the interval id in a ref and clear on unmount.
+ const userOpPollRef = React.useRef<ReturnType<typeof setInterval> | null>(null); const userOperationStatus = setInterval(async () => { // ... }, userOpStatusInterval); + userOpPollRef.current = userOperationStatus;Outside onSend (component scope):
React.useEffect(() => { return () => { if (userOpPollRef.current) clearInterval(userOpPollRef.current); }; }, []);
732-775: Use stable keys for transaction items.Keying by to-address + index is brittle and can collide; prefer transactionName which is guaranteed unique within the kit.
- {transactions.map((transaction, index) => ( - <div key={`${transaction.to}-${index}`}> + {transactions.map((transaction, index) => ( + <div key={transaction.transactionName || `${batchName}-${index}`}>
471-481: Capture the error object in Sentry, not only the message string.Passing the raw error preserves stack/metadata.
- Sentry.captureException( - err instanceof Error ? err.message : 'Error getting userOp status', - { - extra: { - walletAddress: accountAddress, - userOpHash: newUserOpHash, - chainId, - attempts, - }, - } - ); + Sentry.captureException(err, { + extra: { + walletAddress: accountAddress, + userOpHash: newUserOpHash, + chainId, + attempts, + }, + });
575-576: Confirm tagged‐template support or standardize translation callsI found extensive use of both t`key` and t('key') across the repo (e.g. SendModalBatchesTabView at lines 575–576). React-i18next doesn’t natively support tagged templates unless you’re using a custom macro or Babel plugin. Please:
• Verify whether your build config includes a transform (e.g. i18next.macro) that handles t`…`.
• If not, convert all instances of t`…` to the standard function call t('…') to avoid passing an array to t().Example fix in SendModalBatchesTabView.tsx:
- [chainId]: t`error.failedToRemoveBatch`, + [chainId]: t('error.failedToRemoveBatch'),Optional bulk replacement across the file:
rg -l -P "t`[^`]+`" src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx \ | xargs sed -E -i "s#t\`([^`]+)\`#t('\1')#g"src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (5)
2078-2091: Use short, deterministic transaction names (avoid embedding data).Using txData.data in the name can be extremely long and leaks data. Prefer an index-based name derived from current batch length.
- const batchName = `batch-${chainId}`; - const transactionName = `tx-${chainId}-${txData.data}`; + const batchName = `batch-${chainId}`; + const nextIndex = + (kit.getState().batches[batchName]?.length || 0) + 1; + const transactionName = `tx-${chainId}-${nextIndex}`;
581-786: Extract or at least centralize UserOp polling with lifecycle cleanup.The in-component startUserOpPolling (200+ lines) is hard to test and can outlive the component if unmounted mid-poll. Either move to a dedicated hook (preferred), or store interval in a ref and clear on unmount.
Outline:
- Move polling logic into useUserOpPolling hook that accepts params and returns a startPolling function and cleanup.
- Alternatively, stash the interval id in a ref and add a useEffect cleanup in this component as shown in the Batches view comment.
Happy to generate the hook scaffold if you want.
772-783: Capture the original error object in Sentry.Preserve stack/metadata by passing err.
- Sentry.captureException( - err instanceof Error ? err.message : 'Error getting userOp status', - { - extra: { - walletAddress: accountAddress, - userOpHash, - chainId: chainIdForTxHash, - attempts, - }, - } - ); + Sentry.captureException(err, { + extra: { + walletAddress: accountAddress, + userOpHash, + chainId: chainIdForTxHash, + attempts, + }, + });
869-909: Avoid multiple setIsSending(false) calls; rely on finally for consistency.There are multiple early setIsSending(false) calls and a finally with setIsSending(false). Prefer consistently stopping the spinner in finally (except where an early return requires user safety warning flow).
Also applies to: 1198-1202, 1401-1402, 1682-1684, 1943-1945, 1992-1994
1226-1284: Payload single-transaction: gating on selectedAsset may block legitimate payloads.Requiring selectedAsset?.type === 'token' for payload sends couples WalletConnect/payload flows to local UI selections. If the payload defines a valid transaction, consider removing this guard or validating from payload fields only.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
300-304: Chain ID guard looks good and addresses prior comment.Validating chainId !== 0 before constructing/name/batching transactions prevents undefined-chain execution. This aligns with the earlier review note.
__mocks__/useTransactionKit.ts (1)
3-16: Addkit.getProviderstub to the mock
TheuseTransactionKitmock only definesgetProviderat its root, but application code (e.g.src/apps/the-exchange/index.tsx:32) callskit.getProvider(). Without agetProvideronkit, those calls will throw at runtime.Please update
__mocks__/useTransactionKit.tsto include agetProvidermethod underkit, for example:const useTransactionKit = vi.fn(() => ({ kit: { + getProvider: vi.fn(() => { + const provider = { + request: vi.fn(async () => null), + on: vi.fn(), + off: vi.fn(), + removeListener: vi.fn(), + }; + return provider; + }), getState: vi.fn(() => ({ namedTransactions: {}, batches: {}, })), }, - getProvider: vi.fn(() => ({ - request: vi.fn(async () => null), - })), + // Backward-compatible root-level alias + getProvider: vi.fn(() => ({ + request: vi.fn(async () => null), + on: vi.fn(), + off: vi.fn(), + removeListener: vi.fn(), + })), walletAddress: '0x7F30B1960D5556929B03a0339814fE903c55a347', activeChainId: 1, setActiveChainId: vi.fn(), }));
- Verified a call site in
src/apps/the-exchange/index.tsx:32(const provider = kit.getProvider();).src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (1)
86-90: Type mockChains to the real ChainType to keep tests aligned with production typesAnnotate the map with ChainType to catch shape drift early. This was suggested earlier and still applies.
Apply this diff:
+import type { ChainType } from '../../../utils/types'; @@ -const mockChains = { +const mockChains: Record<string, ChainType> = { ethereum: { chainId: 1, chainName: 'Ethereum' }, polygon: { chainId: 137, chainName: 'Polygon' }, arbitrum: { chainId: 42161, chainName: 'Arbitrum' }, };Note: the relative import path is from components/CardsSwap/test to utils: '../../utils/types'.
src/providers/EtherspotTransactionKitProvider.tsx (1)
93-101: Don’t expose setWalletAddress on the public contextThe setter is internal and not part of the type; exposing it invites misuse. Remove it from the context payload.
const contextData = useMemo( () => ({ walletAddress, - setWalletAddress, kit, activeChainId, setActiveChainId, }), [walletAddress, kit, activeChainId] );src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
1971-1994: Differentiate common error types for better UX and telemetryAll errors map to the same generic message. Provide specific messages (rejected, insufficient funds, network) and tag error type in Sentry.
} catch (error: unknown) { - Sentry.captureException(error, { + let errorMessage = t`error.genericSendFailure`; + if (error instanceof Error) { + const msg = error.message.toLowerCase(); + if (msg.includes('reject') || msg.includes('denied')) { + errorMessage = t`error.userRejectedTransaction`; + } else if (msg.includes('insufficient funds')) { + errorMessage = t`error.insufficientFunds`; + } else if (msg.includes('network') || msg.includes('timeout')) { + errorMessage = t`error.networkError`; + } + } + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'send_error', sendId, + errorType: error instanceof Error ? error.name : 'unknown', }, contexts: { send_error: { sendId, error: error instanceof Error ? error.message : String(error), selectedAsset: getAssetSymbol(selectedAsset), amount, recipient, }, }, }); - handleError( - 'Something went wrong while sending the assets, please try again later. If the problem persists, contact the PillarX team for support.' - ); + handleError(errorMessage);
2049-2066: Validate payload transaction recipient before adding to batchCurrently,
txData.tocan become an empty string on invalid payloads, and you proceed to add it to a batch. Validate and abort early.} else if (payload && 'transaction' in payload) { // Single payload transaction batching - const payloadTxAddToBatch = payload.transaction as ITransaction; + const payloadTxAddToBatch = payload.transaction as ITransaction; + if (!payloadTxAddToBatch?.to || !isAddress(payloadTxAddToBatch.to)) { + setErrorMessage(t`error.invalidRecipientAddress`); + return; + } txData = { to: payloadTxAddToBatch?.to || '',src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (2)
507-528: Don’t recreate sendId inside catch; use the same one for traceabilityRecreating sendId makes correlating error logs with the successful path hard. Reuse the sendId created at the top of onSend.
- Sentry.captureException(error, { + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'batch_send_error', - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + sendId, }, contexts: { batch_send_error: { - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + sendId, chainId, batchName, error: error instanceof Error ? error.message : String(error), }, }, });
531-595: Batch removal: add not-found discrimination and keep user feedbackGood addition of user feedback via errorMessage. You can further improve observability by distinguishing a “not found” no-op vs. unexpected errors to reduce noise.
} catch (e) { console.error('Failed to remove batch:', e); const batchTransactions = batches[batchName]; const chainId = batchTransactions?.[0]?.chainId; + const isNotFoundError = + e instanceof Error && /not\s*found|missing batch/i.test(e.message); if (typeof chainId === 'number') { setErrorMessage((prev) => ({ ...prev, - [chainId]: t`error.failedToRemoveBatch`, + [chainId]: isNotFoundError + ? '' // optionally suppress user-facing error if it's benign + : t`error.failedToRemoveBatch`, })); } - Sentry.captureMessage('Failed to remove batch', { - level: 'warning', + Sentry.captureMessage('Failed to remove batch', { + level: isNotFoundError ? 'info' : 'warning', tags: { component: 'send_flow', action: 'remove_batch_failed', removeId, + errorType: isNotFoundError ? 'not_found' : 'unknown', }, contexts: { remove_batch_failed: { removeId, batchName, error: e instanceof Error ? e.message : String(e), }, }, }); }
🧹 Nitpick comments (35)
src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx (5)
111-121: Prefer @testing-library snapshots over react-test-renderer for consistency with DOM environmentUsing react-test-renderer inside a BrowserRouter works in JSDOM, but it’s redundant given you’re already using @testing-library/react. Snapshot via asFragment is simpler and avoids a second rendering stack.
Apply:
-import renderer from 'react-test-renderer'; ... - it('renders correctly and matches snapshot', () => { - const tree = renderer - .create( - <ExchangeTestWrapper> - <EnterAmount type={CardPosition.SWAP} /> - </ExchangeTestWrapper> - ) - .toJSON(); - - expect(tree).toMatchSnapshot(); - }); + it('renders correctly and matches snapshot', () => { + const { asFragment } = render( + <ExchangeTestWrapper> + <EnterAmount type={CardPosition.SWAP} /> + </ExchangeTestWrapper> + ); + expect(asFragment()).toMatchSnapshot(); + });
130-141: Assertion type: toHaveValue expects a string for text inputs; use string or toHaveDisplayValue to avoid flakinessIf the input is type="text" (common for amount inputs), toHaveValue(0.5) can fail because the element value is the string "0.5". Prefer:
- expect(input).toHaveValue(0.5); + expect(input).toHaveValue('0.5');Additionally, consider userEvent for more realistic typing:
-import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; ... - fireEvent.change(input, { target: { value: '0.5' } }); + await userEvent.clear(input); + await userEvent.type(input, '0.5');
130-133: Remove the “test store issues” disclaimer and fix the underlying setupOnce initial state is seeded via a reducer-backed test store (or passed into ExchangeTestWrapper), this comment becomes unnecessary and the test becomes deterministic.
143-145: Redundant vi.clearAllMocks(); keep it in one placeYou already clear mocks in beforeEach. Remove the afterEach duplication.
- afterEach(() => { - vi.clearAllMocks(); - }); + // afterEach not needed; mocks are cleared in beforeEach
95-109: Use a dedicated test store for EnterAmount testsThe current setup dispatches a long list of actions against the global
storeto seed state before each test. In reality, your reducers (including theswapslice) are injected on import viaaddReducerinstore.ts, so “pre-dispatch” isn’t a no-op—you can dispatch before mount without issue. However, relying on the shared store and manual dispatches:
- Risks state leakage between tests
- Introduces a lot of boilerplate in each spec
We recommend an optional refactor to improve test isolation and readability:
- In
test-utils/testUtils.tsx, add a factory for a fresh exchange store:// test-utils/testUtils.tsx import { configureStore } from '@reduxjs/toolkit'; import swapSlice, { initialState as swapInitial } from '../apps/the-exchange/reducer/theExchangeSlice'; // …other slice imports export const makeExchangeTestStore = (preloaded?: Partial<typeof swapInitial>) => configureStore({ reducer: { swap: swapSlice.reducer, // …other reducers }, preloadedState: { swap: { ...swapInitial, ...preloaded }, // …other initial states }, });- Extend
ExchangeTestWrapperto accept a store override:export const ExchangeTestWrapper: React.FC<{ children: React.ReactNode; -}> = ({ children }) => ( + store?: ReturnType<typeof makeExchangeTestStore>; +}> = ({ children, store = defaultStore }) => ( <Provider store={store}> {/* … */} </Provider> );- In your
EnterAmount.test.tsx, replace the manual dispatch block with:- let originalStore = store; - beforeEach(() => { - vi.clearAllMocks(); - store.dispatch(setIsSwapOpen(false)); - …all other dispatches… - }); + let testStore = makeExchangeTestStore({ + isSwapOpen: false, + isReceiveOpen: false, + swapChain: { chainId: 1, chainName: 'Ethereum' }, + receiveChain: { chainId: 137, chainName: 'Polygon' }, + swapToken: mockTokenAssets[0], + receiveToken: mockTokenAssets[1], + amountSwap: 0.1, + amountReceive: 10, + bestOffer: undefined, + searchTokenResult: [], + isOfferLoading: false, + usdPriceSwapToken: 0.1, + }); + beforeEach(() => { + vi.clearAllMocks(); + }); … it('renders correctly', () => { renderer.create( - <ExchangeTestWrapper> + <ExchangeTestWrapper store={testStore}> <EnterAmount …/>This keeps each test self-contained, eliminates repetitive dispatch logic, and leverages a clean, per-test store.
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (4)
260-273: Prefer bigint/viem conversions over ethers.BigNumber for value handling.You can avoid pulling in ethers.BigNumber solely for conversion and standardize on bigint via viem helpers. This reduces bundle weight and keeps types consistent end-to-end.
Apply this diff to compute the value using viem and native bigint:
- if (typeof value === 'bigint') { - // If value is already a native bigint, use it directly - bigIntValue = value; - } else if (value) { - // If value exists but is not a bigint, convert it - bigIntValue = BigNumber.from(value).toBigInt(); - } else { - // If value is undefined/null, use 0 - bigIntValue = BigInt(0); - } + if (typeof value === 'bigint') { + bigIntValue = value; + } else if (typeof value === 'string') { + bigIntValue = value.startsWith('0x') + ? hexToBigInt(value as `0x${string}`) + : BigInt(value); + } else if (typeof value === 'number') { + bigIntValue = BigInt(value); + } else { + bigIntValue = 0n; + }And update imports (outside the selected range):
// replace - import { BigNumber } from 'ethers'; - import { formatEther } from 'viem'; + import { formatEther, hexToBigInt } from 'viem';
299-307: Transaction name derived from raw calldata can be very large and not reliably unique; consider hashing/truncation and adding index/to.Using the full calldata bloats the name and risks collisions across similar steps. Hashing keeps names bounded and deterministic.
Apply this diff:
- const transactionName = `tx-${chainId}-${dataHex}`; + const txIndex = String(i + 1).padStart(2, '0'); + const dataHash = keccak256(dataHex as `0x${string}`).slice(0, 10); + const transactionName = `tx-${chainId}-${txIndex}-${to}-${dataHash}`;Add import (outside the selected range):
import { formatEther, hexToBigInt, keccak256 } from 'viem';
321-328: Use sanitized hex for title detection instead of data.toString().toString() on non-string inputs (e.g., Uint8Array) won’t yield valid hex and will break approve detection. Reuse the sanitized
dataHex.Apply this diff:
- title: getTransactionTitle( - i, - stepTransactions.length, - data?.toString() ?? '' - ), + title: getTransactionTitle( + i, + stepTransactions.length, + dataHex + ),
318-321: Guard against undefined blockchain in description (nit).If either token blockchain is undefined, toUpperCase throws. Minor, but cheap to harden.
Apply this diff:
- const description = `${amountSwap} ${swapToken.symbol} on ${swapToken.blockchain.toUpperCase()} to ${bestOffer.tokenAmountToReceive} ${receiveToken.symbol} on ${receiveToken.blockchain.toUpperCase()}`; + const description = `${amountSwap} ${swapToken.symbol} on ${(swapToken.blockchain || 'unknown').toUpperCase()} to ${bestOffer.tokenAmountToReceive} ${receiveToken.symbol} on ${(receiveToken.blockchain || 'unknown').toUpperCase()}`;src/apps/pillarx-app/components/AnimatedTile/AnimatedTitle.tsx (1)
38-51: Presence event may never fire if address arrives after animation; include accountAddress in depsWith the tightened condition, if accountAddress becomes available after animationComplete is set to true, the effect won’t re-run and recordPresence won’t trigger. Add accountAddress to the dependency array (and drop the eslint-disable) to ensure the event fires once the address is present.
Apply this diff:
useEffect(() => { if (animationComplete && accountAddress) { recordPresence({ address: accountAddress, action: 'app:feed:viewTile', value: { tileId: data.id, tileName: tileDisplay?.title || '', layout: data.layout, }, }); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [animationComplete]); + }, [animationComplete, accountAddress]);If you want to avoid duplicate fires across re-renders, I can follow up with a small useRef guard keyed by tileId and accountAddress. Do you want that?
src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (3)
131-151: Deduplicate chainId computation and reduce repeated callsYou repeatedly compute chainNameToChainIdTokensData(token.blockchain) in this branch. Hoist it into a local const to improve readability and avoid duplicate work.
Apply this diff:
const handleClick = (token: Token) => { + const chainId = chainNameToChainIdTokensData(token.blockchain); if (isSwapOpen) { dispatch(setSwapToken(token)); dispatch( setSwapChain({ - chainId: chainNameToChainIdTokensData(token.blockchain), + chainId, chainName: token.blockchain, }) ); if (accountAddress) { recordPresence({ address: accountAddress, action: 'app:theExchange:sourceTokenSelect', value: { - chainId: chainNameToChainIdTokensData(token.blockchain), + chainId, address: token.contract, symbol: token.symbol, name: token.name, }, }); }
162-173: Deduplicate chainId here as wellSame as for the swap branch, reuse the hoisted chainId for the receive path to keep consistency and reduce duplication.
Apply this diff:
} else { dispatch(setReceiveToken(token)); dispatch( setReceiveChain({ - chainId: chainNameToChainIdTokensData(token.blockchain), + chainId, chainName: token.blockchain, }) ); if (accountAddress) { recordPresence({ address: accountAddress, action: 'app:theExchange:destinationTokenSelect', value: { - chainId: chainNameToChainIdTokensData(token.blockchain), + chainId, address: token.contract, symbol: token.symbol, name: token.name, }, }); }
185-200: Use toLowerCase for address comparisons instead of toLocaleLowerCaseEthereum addresses are ASCII hex; toLowerCase avoids locale-specific transformations that toLocaleLowerCase can introduce.
Apply this diff:
- searchTokenResult[0].contract.toLocaleLowerCase() === - searchToken.toLocaleLowerCase() && + searchTokenResult[0].contract.toLowerCase() === + searchToken.toLowerCase() && receiveToken && - searchTokenResult[0].contract.toLocaleLowerCase() !== - receiveToken.contract.toLocaleLowerCase() + searchTokenResult[0].contract.toLowerCase() !== + receiveToken.contract.toLowerCase()And similarly:
- searchTokenResult[0].contract.toLocaleLowerCase() === - searchToken.toLocaleLowerCase() && + searchTokenResult[0].contract.toLowerCase() === + searchToken.toLowerCase() && swapToken && - searchTokenResult[0].contract.toLocaleLowerCase() !== - swapToken.contract.toLocaleLowerCase() + searchTokenResult[0].contract.toLowerCase() !== + swapToken.contract.toLowerCase()src/providers/GlobalTransactionsBatchProvider.tsx (3)
47-56: Stabilize and sanitize setTransactionMetaForNameValidation is good. Consider trimming inputs and wrapping in useCallback so the function identity is stable for consumers.
Apply this diff:
- const setTransactionMetaForName = ( - transactionName: string, - meta: { title: string; description?: string } - ) => { - if (!transactionName || !meta.title) { - console.warn('Invalid transaction metadata: name and title are required'); - return; - } - setTransactionMeta((prev) => ({ ...prev, [transactionName]: meta })); - }; + const setTransactionMetaForName = React.useCallback( + (transactionName: string, meta: { title: string; description?: string }) => { + const name = transactionName?.trim(); + const title = meta?.title?.trim(); + if (!name || !title) { + console.warn('Invalid transaction metadata: name and title are required'); + return; + } + setTransactionMeta((prev) => ({ + ...prev, + [name]: { ...meta, title }, + })); + }, + [] + );
64-80: Avoid redundant batchCount state updatessetBatchCount runs every tick even when the value hasn’t changed, causing unnecessary renders. Use a functional update to no-op when the count is unchanged.
Apply this diff:
- setBatchCount(Object.keys(batches).length); + setBatchCount((prev) => { + const next = Object.keys(batches).length; + return prev !== next ? next : prev; + });
88-98: Include setTransactionMetaForName in context memo deps (or keep it stable)If setTransactionMetaForName isn’t stable, consumers may receive a stale reference when other deps don’t change. Since we wrapped it with useCallback above, include it in the memo deps.
Apply this diff:
- [walletConnectTxHash, transactionMeta, batchCount] + [walletConnectTxHash, transactionMeta, batchCount, setTransactionMetaForName]src/components/BottomMenu/index.tsx (1)
97-97: Clamp large badge counts to avoid UI overflowIf batchCount spikes, the badge can overflow its container. Clamp to 99 to keep layout stable.
Apply this diff:
- iconNotificationCounter: batchCount, + iconNotificationCounter: Math.min(99, batchCount),src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (2)
130-135: Avoid duplicating useTransactionKit mocks across tests; centralize in test-utilsYou’re mocking useTransactionKit here while ExchangeTestWrapper may already provide a consistent test setup. Centralizing the mock in test-utils reduces drift and maintenance.
Would you like me to move this mock into test-utils and update consumers?
156-167: Snapshot tests can be brittle; prefer focused assertionsConsider keeping snapshot surface small (e.g., specific subtrees) or rely on explicit assertions already present to reduce churn from innocent UI tweaks.
src/hooks/__tests__/useAccountTransactionHistory.test.tsx (1)
29-49: Strengthen the test by asserting setter-induced state changesYou validate initial state and function presence; also verify that calling setters updates the hook state.
Example (add inside the same test):
import { act } from '@testing-library/react'; const { result } = renderHook(() => useAccountTransactionHistory(), { wrapper }); act(() => { result.current.setUserOpStatus('pending' as any); result.current.setTransactionHash('0xabc' as any); result.current.setLatestUserOpInfo({ userOpHash: '0xabc' } as any); result.current.setLatestUserOpChainId(1 as any); }); expect(result.current.userOpStatus).toBe('pending'); expect(result.current.transactionHash).toBe('0xabc'); expect(result.current.latestUserOpInfo).toEqual({ userOpHash: '0xabc' }); expect(result.current.latestUserOpChainId).toBe(1);src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx (2)
1-4: Mock the transaction kit to avoid network/SDK side effects in unit testsThe real EtherspotTransactionKit may attempt network calls (e.g., getWalletAddress). Stub it to keep tests fast and deterministic.
Apply this diff to add the mock before importing the provider:
-import { act, render, screen } from '@testing-library/react'; +import { act, render, screen, waitFor } from '@testing-library/react'; import React from 'react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +// Mock the external SDK before importing the provider +vi.mock('@etherspot/transaction-kit', () => { + class MockKit { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + constructor(_: any) {} + getWalletAddress() { + return Promise.resolve('0xMockWalletAddress'); + } + getEtherspotProvider() { + return { + getChainId: () => 1, + updateConfig: () => {}, + clearAllCaches: () => {}, + }; + } + reset() {} + } + return { default: MockKit, EtherspotTransactionKit: MockKit }; +});
109-118: Assert that setActiveChainId actually updates context stateCurrently, the test only checks that the button remains. Verify that the active chain changes to 5.
Apply this diff:
- // Click the button to change chain - act(() => { - screen.getByTestId('change-chain').click(); - }); - - // The setActiveChainId function should be callable - expect(screen.getByTestId('change-chain')).toBeInTheDocument(); + // Click the button to change chain and verify the update + act(() => { + screen.getByTestId('change-chain').click(); + }); + await waitFor(() => + expect(screen.getByTestId('current-chain')).toHaveTextContent('5') + );src/providers/__tests__/AccountTransactionHistoryProvider.test.tsx (1)
126-169: Reduce test flakiness and duplication for status transitionsYou can tighten these tests and avoid overuse of act/waitFor by asserting synchronously after click, and drive the transitions via a small loop to reduce duplication.
Example refactor:
- // Set to Sending - await act(async () => { - screen.getByTestId('set-sending').click(); - }); - await waitFor(() => { - expect(screen.getByTestId('user-op-status')).toHaveTextContent('Sending'); - }); - // Set to Sent - await act(async () => { - screen.getByTestId('set-sent').click(); - }); - await waitFor(() => { - expect(screen.getByTestId('user-op-status')).toHaveTextContent('Sent'); - }); + const steps = [ + { btn: 'set-sending', expected: 'Sending' }, + { btn: 'set-sent', expected: 'Sent' }, + { btn: 'set-confirmed', expected: 'Confirmed' }, + { btn: 'set-failed', expected: 'Failed' }, + ] as const; + for (const { btn, expected } of steps) { + screen.getByTestId(btn).click(); + expect(screen.getByTestId('user-op-status')).toHaveTextContent(expected); + }__mocks__/EtherspotTransactionKitProvider.tsx (2)
15-26: Prefer null default for Context to catch missing Provider usage in testsProviding a non-null default can mask tests that forget to wrap with the Provider. Consider defaulting to
nulland always wrapping with the mock Provider.-export const EtherspotTransactionKitContext = - createContext<EtherspotTransactionKitContextType | null>({ - data: { - kit: { - // Mock kit methods, can add more as needed for the tests - getWalletAddress: () => Promise.resolve('0xMockWalletAddress'), - }, - walletAddress: '0xMockWalletAddress', - activeChainId: 1, - setActiveChainId: () => {}, - }, - }); +export const EtherspotTransactionKitContext = + createContext<EtherspotTransactionKitContextType | null>(null);
33-49: Memoize the mocked Provider value instead of disabling the lint ruleEven in tests, cheap to make this stable and avoid disabling rules.
-/* eslint-disable react/jsx-no-constructed-context-values */ +// Test-only: memoize the value to avoid unnecessary re-renders @@ -}> = ({ children }) => { - return ( - <EtherspotTransactionKitContext.Provider - value={{ - data: { - kit: { - getWalletAddress: () => Promise.resolve('0xMockWalletAddress'), - }, - walletAddress: '0xMockWalletAddress', - activeChainId: 1, - setActiveChainId: () => {}, - }, - }} - > - {children} - </EtherspotTransactionKitContext.Provider> - ); -}; +}> = ({ children }) => { + const value = React.useMemo( + () => ({ + data: { + kit: { getWalletAddress: () => Promise.resolve('0xMockWalletAddress') }, + walletAddress: '0xMockWalletAddress', + activeChainId: 1, + setActiveChainId: () => {}, + }, + }), + [] + ); + return ( + <EtherspotTransactionKitContext.Provider value={value}> + {children} + </EtherspotTransactionKitContext.Provider> + ); +};src/components/BottomMenuModal/AccountModal.tsx (1)
248-266: Avoid O(n) array recreation when updating failedImages SetSlight perf/readability improvement: clone and add instead of building via Array.concat.
- {!failedImages.has(asset.asset.id) && asset.asset.logo ? ( + {!failedImages.has(asset.asset.id) && asset.asset.logo ? ( <img src={asset.asset.logo} alt={asset.asset.name} - onError={() => - setFailedImages( - (prev) => - new Set(Array.from(prev).concat(asset.asset.id)) - ) - } + onError={() => + setFailedImages(prev => { + const next = new Set(prev); + next.add(asset.asset.id); + return next; + }) + } />src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
2078-2091: Avoid using calldata as part of transactionName; use a bounded, stable idUsing
txData.datain the name can produce extremely long identifiers and leak internals into logs. Prefer a bounded counter or a short hash.- const batchName = `batch-${chainId}`; - const transactionName = `tx-${chainId}-${txData.data}`; + const batchName = `batch-${chainId}`; + // Use a simple per-session counter for uniqueness + const txCounterRef = (SendModalTokensTabView as unknown as { __txCounter?: number }); + txCounterRef.__txCounter = (txCounterRef.__txCounter ?? 0) + 1; + const transactionName = `tx-${chainId}-${txCounterRef.__txCounter}`;
1392-1402: Ensure setIsSending(false) happens after polling kick-off and avoid double-callsYou call
setIsSending(false)immediately after starting polling and also infinally. To prevent flicker and race conditions, rely on thefinally(or a single path) and remove the extra calls.- showHistory(); - setIsSending(false); + showHistory(); @@ - showHistory(); - setIsSending(false); + showHistory(); @@ - showHistory(); - setIsSending(false); + showHistory();Also applies to: 1673-1684, 1943-1971
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (7)
119-143: Remove duplicate in-flight guard in onSendYou already guard against concurrent sends at Lines 91–104. This second guard is redundant and adds noisy Sentry logs and branches that never execute when the first guard is active.
Apply this diff to remove the duplicate check:
- if (isSending[chainId]) { - transactionDebugLog( - 'Another batch is being sent, cannot process the sending of this batch:', - batchName - ); - - Sentry.captureMessage('Batch send disabled - another batch in progress', { - level: 'warning', - tags: { - component: 'send_flow', - action: 'batch_send_disabled', - sendId, - }, - contexts: { - batch_send_disabled: { - sendId, - chainId, - batchName, - isSending: isSending[chainId], - }, - }, - }); - - return; - }
337-483: Avoid overlapping async setInterval ticks; switch to serial setTimeout polling and add cleanupUsing setInterval with an async callback risks overlapping polls if a request takes longer than the interval. Also, the timer isn’t cleared on unmount, which can leak timers.
Apply this diff to serialize polling and avoid overlap:
- const userOperationStatus = setInterval(async () => { - attempts += 1; - try { - const response = await getUserOperationStatus(chainId, newUserOpHash); + let cancelled = false; + let userOperationStatus: ReturnType<typeof setTimeout> | undefined; + const poll = async () => { + attempts += 1; + try { + const response = await getUserOperationStatus(chainId, newUserOpHash); transactionDebugLog(`UserOp status attempt ${attempts}`, response); const status = response?.status; if (status === 'OnChain' && response?.transaction) { setUserOpStatus('Confirmed'); setTransactionHash(response.transaction); transactionDebugLog( 'Transaction successfully submitted on chain with transaction hash:', response.transaction ); Sentry.captureMessage('Batch transaction confirmed on chain', { level: 'info', tags: { component: 'send_flow', action: 'batch_transaction_confirmed', sendId, }, contexts: { batch_transaction_confirmed: { sendId, chainId, batchName, userOpHash: newUserOpHash, transactionHash: response.transaction, attempts, }, }, }); - clearInterval(userOperationStatus); + if (userOperationStatus) clearTimeout(userOperationStatus); return; } const sentryPayload = { walletAddress: accountAddress, userOpHash: newUserOpHash, chainId, transactionHash: response?.transaction, attempts, status, }; if (status === 'Reverted') { if (attempts < maxAttempts) { setUserOpStatus('Sent'); } else { setUserOpStatus('Failed'); transactionDebugLog( 'UserOp Status remained Reverted after 45 sec timeout. Check transaction hash:', response?.transaction ); // Sentry capturing if (!sentryCaptured) { sentryCaptured = true; // Polygon chain if (chainId === 137) { Sentry.captureMessage( `Max attempts reached with userOp status "${status}" on Polygon`, { level: 'warning', extra: sentryPayload } ); } else { // Other chains Sentry.captureException( `Max attempts reached with userOp status "${status}"`, { level: 'error', extra: sentryPayload } ); } } setTransactionHash(response?.transaction); - clearInterval(userOperationStatus); + if (userOperationStatus) clearTimeout(userOperationStatus); } return; } if (['New', 'Pending'].includes(status)) { setUserOpStatus('Sending'); transactionDebugLog( `UserOp Status is ${status}. Check transaction hash:`, response?.transaction ); } if (['Submitted'].includes(status)) { setUserOpStatus('Sent'); transactionDebugLog( `UserOp Status is ${status}. Check transaction hash:`, response?.transaction ); } if (attempts >= maxAttempts) { - clearInterval(userOperationStatus); + if (userOperationStatus) clearTimeout(userOperationStatus); transactionDebugLog( 'Max attempts reached without userOp with OnChain status. Check transaction hash:', response?.transaction ); if (userOpStatus !== 'Confirmed') { setUserOpStatus('Failed'); // Sentry capturing if (!sentryCaptured) { sentryCaptured = true; // Polygon chain if (chainId === 137) { Sentry.captureMessage( `Max attempts reached with userOp status "${status}" on Polygon`, { level: 'warning', extra: sentryPayload } ); } else { // Other chains Sentry.captureException( `Max attempts reached with userOp status "${status}"`, { level: 'error', extra: sentryPayload } ); } } setTransactionHash(response?.transaction); } } } catch (err) { transactionDebugLog('Error getting userOp status:', err); - clearInterval(userOperationStatus); + if (userOperationStatus) clearTimeout(userOperationStatus); setUserOpStatus('Failed'); // Sentry capturing - Sentry.captureException( - err instanceof Error ? err.message : 'Error getting userOp status', - { - extra: { - walletAddress: accountAddress, - userOpHash: newUserOpHash, - chainId, - attempts, - }, - } - ); + Sentry.captureException(err instanceof Error ? err : new Error(String(err)), { + extra: { + walletAddress: accountAddress, + userOpHash: newUserOpHash, + chainId, + attempts, + }, + }); } - }, userOpStatusInterval); + if (!cancelled && attempts < maxAttempts) { + userOperationStatus = setTimeout(poll, userOpStatusInterval); + } + }; + userOperationStatus = setTimeout(poll, userOpStatusInterval);Additionally, add a cleanup on unmount to prevent timer leaks:
// Place near other hooks (outside onSend) const pollingTimersRef = React.useRef<number[]>([]); React.useEffect(() => { return () => { // Clear any outstanding polling timers on unmount pollingTimersRef.current.forEach((id) => clearTimeout(id)); pollingTimersRef.current = []; }; }, []);And when you create a timeout, push its id into pollingTimersRef.current. This ensures no background polling survives component unmounts.
471-482: Pass an Error object to Sentry.captureException, not a stringUsing err.message (string) drops stack traces. Pass the error itself or wrap it.
- Sentry.captureException( - err instanceof Error ? err.message : 'Error getting userOp status', - { - extra: { - walletAddress: accountAddress, - userOpHash: newUserOpHash, - chainId, - attempts, - }, - } - ); + Sentry.captureException(err instanceof Error ? err : new Error(String(err)), { + extra: { + walletAddress: accountAddress, + userOpHash: newUserOpHash, + chainId, + attempts, + }, + });
193-220: Make estimated cost formatting consistent and reuse the computed value in success logsYou compute an estimated string with nativeAsset.decimals, but later log with a hard-coded 18 decimals. This creates inconsistent telemetry.
Apply these diffs:
- Initialize holders for reuse (at the start of the try block):
- try { + try { + let estimatedCostStr: string | null = null; + let estimatedCostSymbol: string | null = null;
- When you compute the estimate, populate those holders:
- if (estimatedCostBN) { + if (estimatedCostBN) { const nativeAsset = getNativeAssetForChainId( batchEst.transactions[0].chainId as number ); const estimatedCost = ethers.utils.formatUnits( estimatedCostBN, nativeAsset.decimals ); transactionDebugLog('Transaction batch estimated cost:', estimatedCost); - setEstimatedCostFormatted((prev) => ({ - ...prev, - [chainId]: `${formatAmountDisplay(estimatedCost, 0, 6)} ${nativeAsset.symbol}`, - })); + const formatted = `${formatAmountDisplay(estimatedCost, 0, 6)} ${nativeAsset.symbol}`; + estimatedCostStr = formatted; + estimatedCostSymbol = nativeAsset.symbol; + setEstimatedCostFormatted((prev) => ({ ...prev, [chainId]: formatted }));
- Reuse the same value in the success Sentry context:
- batch_send_success: { + batch_send_success: { sendId, chainId, batchName, - estimatedCost: estimatedCostBN - ? ethers.utils.formatUnits(estimatedCostBN, 18) - : null, + estimatedCost: estimatedCostStr, + estimatedCostSymbol, groupedTransactionsCount: groupedBatchesByChainId[chainId]?.length || 0, },Also applies to: 487-506
73-87: Memoization may go stale if kit mutates batches in placegroupedBatchesByChainId depends on object identity of batches. If the kit mutates the batches object/arrays in place (common in state machines), the memo won’t recompute and you’ll render stale grouping despite triggerUpdate.
Options:
- Simplest: drop useMemo and recompute per render (lists are small in a modal).
- Or broaden dependencies to include keys/lengths:
React.useMemo(() => compute(), [batches, Object.keys(batches).join(','), JSON.stringify(Object.values(batches).map((a) => a.length))]).- Best: if the kit exposes a subscribe/onStateChange, use it to mirror state to local component state rather than forceUpdate hacks. I can help wire this if you confirm the kit API.
193-207: Batch estimate uses the first transaction’s cost onlyYou’re displaying the cost of transactions[0].cost, which can undercount for multi-tx batches. If the kit doesn’t expose a batch-level estimate, consider summing tx costs.
Would you like me to update this to sum available tx.cost values or to pick up a batch-level total if the kit exposes one (e.g., batchEst.totalCost)?
65-71: Force re-rendering to reflect kit state is okay; consider a subscription for robustnessUsing a dummy state to force refresh works. If the kit provides a state subscription, prefer subscribing to updates to keep “batches” in sync without manual triggerUpdate calls.
I can help integrate a subscribe/unsubscribe pattern if you point me to the kit’s state change API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
src/apps/the-exchange/components/CardsSwap/test/__snapshots__/CardSwap.test.tsx.snapis excluded by!**/*.snapsrc/apps/the-exchange/components/EnterAmount/test/__snapshots__/EnterAmount.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (29)
__mocks__/EtherspotTransactionKitProvider.tsx(1 hunks)__mocks__/useTransactionKit.ts(1 hunks)src/apps/pillarx-app/components/AnimatedTile/AnimatedTitle.tsx(1 hunks)src/apps/pillarx-app/index.tsx(3 hunks)src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx(4 hunks)src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx(4 hunks)src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx(2 hunks)src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx(4 hunks)src/apps/the-exchange/components/SelectDropdown/SelectDropdown.tsx(5 hunks)src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx(3 hunks)src/apps/token-atlas/components/HeaderSearch/HeaderSeach.tsx(2 hunks)src/apps/token-atlas/components/SelectChainDropdown/SelectChainDropdown.tsx(3 hunks)src/apps/token-atlas/components/TokensSearchInput/TokensSearchInput.tsx(3 hunks)src/apps/token-atlas/components/TokensSlider/TokensSlider.tsx(3 hunks)src/components/AppsList.tsx(3 hunks)src/components/BottomMenu/index.tsx(2 hunks)src/components/BottomMenuModal/AccountModal.tsx(3 hunks)src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx(12 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(23 hunks)src/components/BottomMenuModal/SendModal/index.tsx(2 hunks)src/containers/Authorized.tsx(2 hunks)src/hooks/__tests__/useAccountTransactionHistory.test.tsx(2 hunks)src/providers/EtherspotTransactionKitProvider.tsx(1 hunks)src/providers/GlobalTransactionsBatchProvider.tsx(3 hunks)src/providers/__tests__/AccountTransactionHistoryProvider.test.tsx(1 hunks)src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx(1 hunks)src/translations/en.json(1 hunks)src/types/blockchain.ts(1 hunks)src/utils/blockchain.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/apps/token-atlas/components/SelectChainDropdown/SelectChainDropdown.tsx
- src/types/blockchain.ts
- src/apps/token-atlas/components/TokensSlider/TokensSlider.tsx
- src/apps/token-atlas/components/HeaderSearch/HeaderSeach.tsx
- src/components/AppsList.tsx
- src/apps/the-exchange/components/SelectDropdown/SelectDropdown.tsx
- src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx
- src/apps/token-atlas/components/TokensSearchInput/TokensSearchInput.tsx
- src/components/BottomMenuModal/SendModal/index.tsx
- src/apps/pillarx-app/index.tsx
- src/containers/Authorized.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
PR: pillarwallet/x#275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsxsrc/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
PR: pillarwallet/x#320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx
🧬 Code Graph Analysis (12)
src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx (2)
__mocks__/EtherspotTransactionKitProvider.tsx (2)
EtherspotTransactionKitProvider(28-49)EtherspotTransactionKitContext(15-26)src/providers/EtherspotTransactionKitProvider.tsx (2)
EtherspotTransactionKitProvider(31-109)EtherspotTransactionKitContext(23-24)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (6)
src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/utils/blockchain.ts (2)
getNativeAssetForChainId(69-141)buildTransactionData(357-415)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/utils/common.ts (1)
transactionDescription(62-75)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/types/blockchain.ts (1)
ITransaction(22-27)
src/providers/__tests__/AccountTransactionHistoryProvider.test.tsx (1)
src/providers/AccountTransactionHistoryProvider.tsx (1)
AccountTransactionHistoryContext(28-29)
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (3)
src/utils/blockchain.ts (2)
getChainName(260-279)getNativeAssetForChainId(69-141)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
__mocks__/EtherspotTransactionKitProvider.tsx (1)
src/providers/EtherspotTransactionKitProvider.tsx (3)
EtherspotTransactionKitContextType(14-21)EtherspotTransactionKitContext(23-24)EtherspotTransactionKitProvider(31-109)
src/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (3)
src/store.ts (1)
store(65-77)src/apps/the-exchange/reducer/theExchangeSlice.ts (12)
setAmountSwap(79-81)setAmountReceive(82-84)setBestOffer(85-87)setIsOfferLoading(97-99)setIsSwapOpen(61-63)setIsReceiveOpen(64-66)setSwapChain(67-69)setReceiveChain(70-72)setSwapToken(73-75)setReceiveToken(76-78)setSearchTokenResult(88-90)setUsdPriceSwapToken(91-93)src/test-utils/testUtils.tsx (1)
ExchangeTestWrapper(154-168)
src/components/BottomMenuModal/AccountModal.tsx (3)
src/utils/logout.ts (1)
useComprehensiveLogout(8-40)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/utils/blockchain.ts (2)
getLogoForChainId(158-192)getChainName(260-279)
src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (1)
src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)
src/utils/blockchain.ts (1)
src/apps/the-exchange/utils/wrappedTokens.ts (1)
isNativeToken(30-31)
src/providers/EtherspotTransactionKitProvider.tsx (1)
__mocks__/EtherspotTransactionKitProvider.tsx (3)
EtherspotTransactionKitContextType(5-13)EtherspotTransactionKitContext(15-26)EtherspotTransactionKitProvider(28-49)
src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx (4)
src/store.ts (1)
store(65-77)src/apps/the-exchange/reducer/theExchangeSlice.ts (12)
setIsSwapOpen(61-63)setIsReceiveOpen(64-66)setSwapChain(67-69)setReceiveChain(70-72)setSwapToken(73-75)setReceiveToken(76-78)setAmountSwap(79-81)setAmountReceive(82-84)setBestOffer(85-87)setSearchTokenResult(88-90)setIsOfferLoading(97-99)setUsdPriceSwapToken(91-93)src/apps/token-atlas/reducer/tokenAtlasSlice.ts (1)
setSearchTokenResult(74-76)src/test-utils/testUtils.tsx (1)
ExchangeTestWrapper(154-168)
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (1)
src/services/tokensData.ts (1)
chainNameToChainIdTokensData(234-255)
🔇 Additional comments (13)
src/apps/the-exchange/components/EnterAmount/test/EnterAmount.test.tsx (2)
114-116: Good move to use ExchangeTestWrapper for provider/router/theme contextsWrapping EnterAmount with ExchangeTestWrapper is the right call and removes boilerplate in tests. This also aligns with app wiring.
95-109: Previous issue about “calling action creators directly” is resolvedGood fix switching to store.dispatch(...) for setup. Assuming reducer registration is addressed, state will now update as intended.
src/apps/the-exchange/components/ExchangeAction/ExchangeAction.tsx (2)
65-66: Good adoption of Transaction Kit and per-transaction metadata.Switching to useTransactionKit and setTransactionMetaForName aligns with the new TK2 flow and improves traceability per tx.
299-317: Chain ID derivation is correct—no per-step cross-chain flowsgetStepTransactions returns only same-chain steps (it doesn’t include a per-step
chainIdfield), and all transactions are run onswapToken.blockchain. There are no bridge or multi-chain flows in this hook, so deriving the chain ID fromswapTokenfor each step is appropriate.Likely an incorrect or invalid review comment.
src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx (2)
35-36: Good migration to local transaction kit and address gatingSwitching to useTransactionKit and deriving accountAddress locally is aligned with the kit v2 migration. This also enables consistent address gating elsewhere in the app.
Also applies to: 67-67
180-205: Dependency selection aligns with prior learning on searchTokenNot including searchToken in this effect’s dependencies is correct here, given swapTokenList/receiveTokenList derive from search results. Matches our prior learning for this component.
src/translations/en.json (1)
53-58: New error keys look good; placeholder usage is consistentThe added error messages and the trailing comma fix are valid. The chain-scoped variant correctly uses the {{chainName}} placeholder.
If you maintain other locales, ensure these keys are added there too. I can generate a quick search to find usages of these keys to help update tests or UI if needed.
src/providers/GlobalTransactionsBatchProvider.tsx (1)
58-86: Overall: Solid integration with kit state and periodic pruningThe interval-based prune keyed off kit.getState() is pragmatic, and the mounted guard prevents leaks. The metadata map approach keeps concerns separated from kit internals.
src/components/BottomMenu/index.tsx (1)
40-40: LGTM: now sourcing a reactive batch counter from the providerSwitching to useGlobalTransactionsBatch().batchCount addresses the previous stale getState() snapshot issue. Assuming the provider polls/subscribes centrally, this should keep the badge in sync.
src/utils/blockchain.ts (1)
357-415: Verify and update allbuildTransactionDatacall sites for the newamount/valuetypesThe proposed change breaks callers that currently pass a
numberforamount(and expectvalueto be abigintor string). Please update these invocations to supply either a human-readable string or a base-unitbigint, for example withamount.toString()orBigInt(parseUnits(...)), and ensure the returnedvalueis always abigint.Call sites to update:
- src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx:1459
- src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx:1715
- src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx:2026
Make sure each call now passes:
amountasstring | bigint- Consumes the returned
value: bigintcorrectly downstreamsrc/apps/the-exchange/components/CardsSwap/test/CardSwap.test.tsx (1)
141-154: LGTM: store-driven test setup is correctDispatching to the shared store before rendering ensures the component sees the intended state. Good isolation with beforeEach.
src/providers/EtherspotTransactionKitProvider.tsx (1)
43-55: No lingeringkit/kitRefdependencies — safe to stabilize the instance
A scan ofsrc/providers/EtherspotTransactionKitProvider.tsxshows exactly two effects (one onactiveChainId, one onkit) and no otherkitRefusages anywhere insrc. You can confidently apply the refactor for lines 43–55 (and the analogous changes at 57–83, 85–92) as originally proposed.src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (1)
163-191: Overall send flow: good structure and error pathsEstimation + send + early userOpHash validation with explicit user feedback and Sentry breadcrumbs looks solid.
Also applies to: 241-266, 281-307
| try { | ||
| kit.name({ transactionName }); // select the transaction | ||
| kit.remove(); // remove the selected transaction | ||
| triggerUpdate(); | ||
|
|
There was a problem hiding this comment.
Scope removal to the target batch to avoid deleting the wrong transaction
Calling kit.name(...).remove() without scoping to the batch can remove a transaction with the same name in another batch. Scope by batchName.
- try {
- kit.name({ transactionName }); // select the transaction
- kit.remove(); // remove the selected transaction
- triggerUpdate();
+ try {
+ // Select scope by batch, then by transaction name, then remove
+ kit.batch({ batchName }).name({ transactionName }).remove();
+ triggerUpdate();📝 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 { | |
| kit.name({ transactionName }); // select the transaction | |
| kit.remove(); // remove the selected transaction | |
| triggerUpdate(); | |
| try { | |
| // Select scope by batch, then by transaction name, then remove | |
| kit.batch({ batchName }).name({ transactionName }).remove(); | |
| triggerUpdate(); |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx around
lines 615 to 619, the code calls kit.name({ transactionName }); kit.remove();
which can delete a transaction with the same name in a different batch; scope
the selection to the intended batch by including the batch identifier when
selecting before removing (for example, call kit.name({ transactionName,
batchName }) or call kit.where({ batchName }).name(transactionName) then
kit.remove()), then call triggerUpdate() as before so only the transaction in
the target batch is deleted.
There was a problem hiding this comment.
I think that's a misunderstanding for TxKit2 from CodeRabbit @RanaBug?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| // Validate decimals | ||
| if (decimals < 0 || decimals > 18 || !Number.isInteger(decimals)) { | ||
| throw new Error('Invalid decimals: must be an integer between 0 and 18'); |
There was a problem hiding this comment.
Apparently you can have 77 decimals! But let's leave it at 18 - never seen a use case for anything bigger than 18.
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
This LGTM, this represents another massive chunk of technical debt that has been resolved 🚀 just need to have a chat about that logic that is confusing me 😂. Great work!
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
github-actions
bot
temporarily deployed
to
Preview (feat/PRO-3525/integration-transaction-kit-2)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (13)
src/providers/EtherspotTransactionKitProvider.tsx (3)
65-74: Remove setWalletAddress from the public context payload (not part of the type and unused by consumers)You expose
setWalletAddressincontextData, but it isn’t declared inEtherspotTransactionKitContextTypeand repo-wide usage shows no external consumers. Exposing internal setters increases surface area without benefit.const contextData = useMemo( () => ({ walletAddress, - setWalletAddress, kit, activeChainId, setActiveChainId, }), - [walletAddress, kit, activeChainId] + [walletAddress, kit, activeChainId] );
1-1: Re-enable jsx-no-constructed-context-values; memoize the Provider valueYou already memoize
contextData, but you still construct{ data: contextData }inline and disabled the lint rule. Memoizing the outer value avoids avoidable re-renders of all consumers.-/* eslint-disable react/jsx-no-constructed-context-values */ @@ const contextData = useMemo( @@ ); + const contextValue = useMemo( + () => ({ data: contextData }), + [contextData] + ); + return ( - <EtherspotTransactionKitContext.Provider value={{ data: contextData }}> + <EtherspotTransactionKitContext.Provider value={contextValue}> {children} </EtherspotTransactionKitContext.Provider> );Also applies to: 76-80
51-55: Avoid re-instantiating EtherspotTransactionKit; keep a single stable instance, update chainId in-place, and add cleanupRecreating the kit when
kitConfigchanges will drop connections, lose pending state, and can leak listeners. Maintain one instance viauseRef, update its chain with an in-place method if available, and clean up on unmount. Also harden the wallet address fetch with cancellation and error handling.Apply this refactor:
@@ - const [walletAddress, setWalletAddress] = useState<string>(); - const kitRef = useRef<EtherspotTransactionKit | null>(null); + const [walletAddress, setWalletAddress] = useState<string>(); + // Create a single instance once; keep it stable across renders. + const kitRef = useRef<EtherspotTransactionKit>( + new EtherspotTransactionKit(config) + ); @@ - // If activeChainId is provided, override the chainId in config - // Setting an activeChainId will allow the kit to use the correct chain for single transaction management - // For batches management, the chainId will be set based on the transactions being added to the batch - const kitConfig = useMemo( - () => ({ - ...config, - chainId: activeChainId ?? config.chainId, - }), - [config, activeChainId] - ); + // Keep a single kit instance; switch its chain in-place when needed. @@ - const kit = useMemo(() => { - const newKit = new EtherspotTransactionKit(kitConfig); - kitRef.current = newKit; - return newKit; - }, [kitConfig]); + const kit = kitRef.current; + + // Update chain in-place if the kit supports it. + useEffect(() => { + const maybeSetChainId = (kitRef.current as any)?.setChainId; + if (activeChainId && typeof maybeSetChainId === 'function') { + maybeSetChainId.call(kitRef.current, activeChainId); + } + }, [activeChainId]); + + // Cleanup on unmount (guard for different SDK versions). + useEffect(() => { + return () => { + (kitRef.current as any)?.destroy?.(); + (kitRef.current as any)?.disconnect?.(); + }; + }, []); @@ - useEffect(() => { - const init = async () => { - const address = await kit.getWalletAddress(); - if (address) setWalletAddress(address); - }; - init(); - }, [kit]); + useEffect(() => { + let cancelled = false; + (async () => { + try { + const address = await kitRef.current.getWalletAddress(); + if (!cancelled && address) setWalletAddress(address); + } catch (e) { + // eslint-disable-next-line no-console + console.error('EtherspotTransactionKitProvider: getWalletAddress failed', e); + } + })(); + return () => { + cancelled = true; + }; + }, [activeChainId]);Also applies to: 43-49, 38-38, 57-64
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (7)
1933-1955: Differentiate user-facing errors by type for better UX and telemetryThe catch-all shows a generic message. Distinguish user rejection, insufficient funds, and network timeouts for clarity; include errorType tag in Sentry.
- } catch (error: unknown) { - Sentry.captureException(error, { + } catch (error: unknown) { + let errorMessage = t`error.genericSendFailure`; + let errorType = 'unknown'; + if (error instanceof Error) { + const msg = error.message.toLowerCase(); + if (msg.includes('rejected') || msg.includes('denied')) { + errorMessage = t`error.userRejectedTransaction`; + errorType = 'user_rejected'; + } else if (msg.includes('insufficient funds')) { + errorMessage = t`error.insufficientFunds`; + errorType = 'insufficient_funds'; + } else if (msg.includes('network') || msg.includes('timeout')) { + errorMessage = t`error.networkError`; + errorType = 'network'; + } + } + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'send_error', sendId, + errorType, }, @@ - handleError( - 'Something went wrong while sending the assets, please try again later. If the problem persists, contact the PillarX team for support.' - ); + handleError(errorMessage);
967-1122: Deeply nested payload batches flow — extract helpers to reduce complexityThe batches loop contains build/estimate/validate/cost-check/send/poll logic inline, making it hard to maintain and test.
Consider extracting:
- buildBatchTransactions(...)
- validateBatchEstimation(...)
- checkBatchCostWarnings(...)
- sendAndHandleBatches(...)
I can provide concrete helper implementations if you want to proceed.
623-788: Poller leaks intervals and relies on stale outer state; add cleanup and avoid stale readsstartUserOpPolling sets an interval but does not register cleanup on unmount and reads userOpStatus from outer scope inside the interval. Store interval IDs in a ref and clear them on unmount; avoid decisions based on possibly-stale outer userOpStatus.
Apply:
- import React, { useEffect, useMemo } from 'react'; + import React, { useEffect, useMemo, useRef } from 'react'; @@ + // Track active polling intervals for cleanup + const pollingIntervalsRef = useRef<number[]>([]); @@ - const userOperationStatus = setInterval(async () => { + const userOperationStatus = window.setInterval(async () => { attempts += 1; try { const response = await getUserOperationStatus( chainIdForTxHash, userOpHash ); @@ - if (attempts >= maxAttempts) { - clearInterval(userOperationStatus); + if (attempts >= maxAttempts) { + clearInterval(userOperationStatus); transactionDebugLog( 'Max attempts reached without userOp with OnChain status. Check transaction hash:', response?.transaction ); - if (userOpStatus !== 'Confirmed') { - setUserOpStatus('Failed'); - // Sentry capturing ... - } + setUserOpStatus('Failed'); + // Sentry capturing ... } @@ - clearInterval(userOperationStatus); + clearInterval(userOperationStatus); setUserOpStatus('Failed'); // Sentry capturing ... } - }, userOpStatusInterval); + }, userOpStatusInterval); + pollingIntervalsRef.current.push(userOperationStatus); } + + // Cleanup all polling intervals on unmount + useEffect(() => { + return () => { + for (const id of pollingIntervalsRef.current) { + clearInterval(id); + } + pollingIntervalsRef.current = []; + }; + }, []);
534-547: Do not display a token address as the fee “symbol”; carry and use the real symbolhandleEstimation formats the paymaster fee asset using selectedFeeAsset.token?.toUpperCase(), which is the address, not the symbol. Persist symbol when setting selectedFeeAsset and render that here.
- symbol = selectedFeeAsset.token?.toUpperCase() || ''; + symbol = selectedFeeAsset.symbol || 'ERC20';Additionally required outside this hunk:
- Extend the selectedFeeAsset state shape and assignments:
// state - const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ - token: string; - decimals: number; - tokenPrice?: string; - balance?: string; - }>(); + const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ + token: string; + decimals: number; + tokenPrice?: string; + balance?: string; + symbol?: string; + }>(); // when defaulting (around lines 305-313) setSelectedFeeAsset({ token: feeOptions[0].asset.contract, decimals: feeOptions[0].asset.decimals, tokenPrice: feeOptions[0].asset.price?.toString(), balance: feeOptions[0].value?.toString(), + symbol: feeOptions[0].asset.symbol, }); // in handleOnChange (around lines 2123-2128) setSelectedFeeAsset({ token: tokenAddress, decimals: Number(values[3]) ?? 18, tokenPrice: tokenOption.asset.price?.toString(), balance: tokenOption.value?.toString(), + symbol: tokenOption.asset.symbol, });
1484-1492: Guard approval tx when approveData is missing to avoid failing batchesApproval is added unconditionally. If approveData is empty, estimation/send will fail ambiguously.
- // 1. Approval transaction - kit - .transaction({ - chainId: selectedAsset.chainId, - to: selectedFeeAsset.token, - value: '0', - data: approveData, - }) - .name({ transactionName: 'approve' }) - .addToBatch({ batchName }); + // 1. Approval transaction (only if precomputed) + if (!approveData) { + handleError(t`error.approvalDataUnavailable`); + return; + } + kit + .transaction({ + chainId: selectedAsset.chainId, + to: selectedFeeAsset.token, + value: '0', + data: approveData, + }) + .name({ transactionName: 'approve' }) + .addToBatch({ batchName });
605-606: Latest userOp chain ID should use the queried chain, not selectedAsset.chainIdWhen polling, setLatestUserOpChainId should reflect the chain you query for the tx hash, i.e., chainIdForTxHash. Using selectedAsset?.chainId is wrong for payload flows and can be wrong after chain switches.
- setLatestUserOpChainId(selectedAsset?.chainId); + setLatestUserOpChainId(chainIdForTxHash);
2018-2034: Validate payload batching “to” address before adding to batchWhen batching a single payload transaction, you permit to: '' and skip address validation. This can enqueue an invalid transaction silently.
- const payloadTxAddToBatch = payload.transaction as ITransaction; + const payloadTxAddToBatch = payload.transaction as ITransaction; + if (!payloadTxAddToBatch?.to || !isAddress(payloadTxAddToBatch.to)) { + setErrorMessage(t`error.invalidRecipientAddress`); + return; + } txData = { to: payloadTxAddToBatch?.to || '',src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (3)
320-337: Consider extracting the batch UserOp poller into a shared hookBoth tabs implement similar polling. A shared useUserOpPolling hook would reduce duplication and centralize Sentry/status handling.
If helpful, I can scaffold the hook with a minimal API to cover both single and batch flows.
616-619: Scope removal to the target batch to avoid deleting the wrong transactionkit.name(...).remove() can remove a same-named transaction in a different batch. Scope selection to batch, then remove.
- kit.name({ transactionName }); // select the transaction - kit.remove(); // remove the selected transaction + kit.batch({ batchName }).name({ transactionName }).remove(); triggerUpdate();
509-529: Reuse the same sendId in the catch block for coherent traceabilityThe catch block regenerates a new sendId, breaking Sentry correlation. Use the existing sendId variable.
- Sentry.captureException(error, { + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'batch_send_error', - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + sendId, }, contexts: { batch_send_error: { - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + sendId, chainId, batchName, error: error instanceof Error ? error.message : String(error), }, }, });
🧹 Nitpick comments (7)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
1265-1271: Normalize payload value to a BigNumberish consistentlyvalue is currently a string; use the existing safeBigIntConversion for consistency with other paths.
- .transaction({ + .transaction({ chainId: chainIdToUse, to: txData.to, - value: txData.value, + value: safeBigIntConversion(txData.value), data: txData.data, })
2046-2056: Transaction names should be short and stable; avoid embedding full data blobstransactionName uses txData.data (potentially long hex) which harms readability and can collide across batches. Prefer a short slug using to + a hash of data.
- const transactionName = `tx-${chainId}-${txData.data}`; + const transactionName = `tx-${chainId}-${txData.to}-${(txData.data || '0x').slice(0,10)}`; + // Alternatively: use keccak256(txData.data) if available for compact uniquenessIf you’d like, I can wire keccak256 from viem to produce a 6–8 char suffix.
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (5)
12-12: Import useRef/useEffect for poll cleanupWe’ll introduce a polling intervals ref and component cleanup; import the hooks now.
-import React from 'react'; +import React, { useEffect, useRef } from 'react';
91-105: Duplicate in-progress checks block; remove the second to simplify control flowYou guard isSending[chainId] twice with near-identical telemetry. Keep the early-return block and remove the later duplicate to avoid drift.
@@ - if (isSending[chainId]) { - transactionDebugLog( - 'Another batch is being sent, cannot process the sending of this batch:', - batchName - ); - Sentry.captureMessage('Batch send disabled - another batch in progress', { - level: 'warning', - tags: { - component: 'send_flow', - action: 'batch_send_disabled', - sendId, - }, - contexts: { - batch_send_disabled: { - sendId, - chainId, - batchName, - isSending: isSending[chainId], - }, - }, - }); - return; - } + // removed duplicate in-progress guardAlso applies to: 120-144
195-208: Prefer viem’s formatUnits and drop ethers import for consistency and bundle sizeElsewhere you use viem; here you import ethers only to format units. Use formatUnits from viem and remove the ethers import.
-import { ethers } from 'ethers'; +import { formatUnits } from 'viem'; @@ - const estimatedCost = ethers.utils.formatUnits( - estimatedCostBN, - nativeAsset.decimals - ); + const estimatedCost = formatUnits(estimatedCostBN, nativeAsset.decimals);Also update Sentry context later (see below).
338-485: Add interval cleanup and avoid stale reads in the batch pollerLike the tokens tab, the batch poller doesn’t clear intervals on unmount and reads outer userOpStatus. Track interval IDs in a ref, clear them on unmount, and avoid checks against possibly-stale outer state.
+ // Track active polling intervals for cleanup + const pollingIntervalsRef = useRef<number[]>([]); @@ - const userOperationStatus = setInterval(async () => { + const userOperationStatus = window.setInterval(async () => { attempts += 1; @@ - if (attempts >= maxAttempts) { - clearInterval(userOperationStatus); + if (attempts >= maxAttempts) { + clearInterval(userOperationStatus); transactionDebugLog( 'Max attempts reached without userOp with OnChain status. Check transaction hash:', response?.transaction ); - if (userOpStatus !== 'Confirmed') { - setUserOpStatus('Failed'); - // Sentry capturing ... - } + setUserOpStatus('Failed'); + // Sentry capturing ... } @@ - }, userOpStatusInterval); + }, userOpStatusInterval); + pollingIntervalsRef.current.push(userOperationStatus); @@ + // Cleanup all polling intervals on unmount + useEffect(() => { + return () => { + for (const id of pollingIntervalsRef.current) { + clearInterval(id); + } + pollingIntervalsRef.current = []; + }; + }, []);
497-505: Use consistent decimals when logging estimatedCost to SentryHere you format with 18 decimals regardless of chain, but above you used nativeAsset.decimals for UI. Keep them consistent.
- estimatedCost: estimatedCostBN - ? ethers.utils.formatUnits(estimatedCostBN, 18) + estimatedCost: estimatedCostBN + ? formatUnits(estimatedCostBN, getNativeAssetForChainId(chainId).decimals) : null,Note: This assumes you applied the earlier switch to viem.formatUnits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/apps/leaderboard/components/LeaderboardTab/LeaderboardTab.tsx(2 hunks)src/apps/leaderboard/components/LeaderboardTab/test/LeaderboardTab.test.tsx(6 hunks)src/apps/leaderboard/components/PxPointsSummary/PxPointsSummary.tsx(2 hunks)src/apps/leaderboard/components/PxPointsSummary/tests/PxPointsSummary.test.tsx(4 hunks)src/apps/leaderboard/index.tsx(2 hunks)src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx(13 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(21 hunks)src/containers/Authorized.tsx(2 hunks)src/providers/EtherspotTransactionKitProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/apps/leaderboard/index.tsx
- src/apps/leaderboard/components/LeaderboardTab/LeaderboardTab.tsx
- src/apps/leaderboard/components/PxPointsSummary/PxPointsSummary.tsx
- src/containers/Authorized.tsx
- src/apps/leaderboard/components/LeaderboardTab/test/LeaderboardTab.test.tsx
- src/apps/leaderboard/components/PxPointsSummary/tests/PxPointsSummary.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.864Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
📚 Learning: 2025-08-20T09:14:16.864Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.864Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsxsrc/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsxsrc/providers/EtherspotTransactionKitProvider.tsx
🧬 Code graph analysis (3)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (6)
src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/utils/blockchain.ts (3)
getNativeAssetForChainId(69-141)safeBigIntConversion(358-366)buildTransactionData(368-474)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/utils/common.ts (1)
transactionDescription(62-75)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/types/blockchain.ts (1)
ITransaction(22-27)
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (3)
src/utils/blockchain.ts (2)
getChainName(260-279)getNativeAssetForChainId(69-141)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
src/providers/EtherspotTransactionKitProvider.tsx (1)
__mocks__/EtherspotTransactionKitProvider.tsx (3)
EtherspotTransactionKitContextType(5-13)EtherspotTransactionKitContext(15-26)EtherspotTransactionKitProvider(28-49)
🪛 Biome (2.1.2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
[error] 14-14: Do not shadow the global "isNaN" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/providers/EtherspotTransactionKitProvider.tsx (2)
34-36: Confirm intended behavior when config.chainId changes after mount
activeChainIdis initialized fromconfig.chainIdonce and then decoupled. Ifconfig.chainIdcan change at runtime (e.g., environment switch, re-auth), you may want to resync when the user hasn’t explicitly selected a chain.If resync is desired, consider:
+ useEffect(() => { + setActiveChainId((prev) => + prev === undefined ? config.chainId : prev + ); + }, [config.chainId]);If decoupling is intentional, no change needed—just confirm.
23-29: Context and provider surface look goodClear, typed context payload with just what consumers need. Aligns with the repo’s pattern of lifting providers to the container level (per our last learning). No changes required here.
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
1969-1976: Good guard: prevent batching for WalletConnect flowsBlocking batching when payload is a WalletConnect request avoids unhandled flows and mis-sends. Nice.
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (1)
69-88: Nice: grouping batches by chainId and guarding undefined chainsThe grouping logic is straightforward and skips invalid/empty batches. This keeps the UI resilient.
| const chainIdToUse = payloadTx?.chainId; | ||
|
|
||
| // Set the active chain context for this transaction | ||
| // This ensures the transaction is built and sent on the correct chain | ||
| setActiveChainId(chainIdToUse); | ||
|
|
||
| kit | ||
| .transaction({ | ||
| chainId: chainIdToUse, | ||
| to: txData.to, | ||
| value: txData.value, | ||
| data: txData.data, | ||
| }) | ||
| .name({ transactionName: 'tx-payload-single-send' }); | ||
|
|
There was a problem hiding this comment.
Validate payload chainId before building/sending; avoid undefined chain context
chainIdToUse can be undefined in payload flows. Passing undefined into setActiveChainId and kit.transaction will break at runtime.
- const chainIdToUse = payloadTx?.chainId;
+ const chainIdToUse = payloadTx?.chainId ?? etherspotDefaultChainId;
+ if (typeof chainIdToUse !== 'number') {
+ handleError(t`error.invalidChainIdForTransaction`);
+ 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 chainIdToUse = payloadTx?.chainId; | |
| // Set the active chain context for this transaction | |
| // This ensures the transaction is built and sent on the correct chain | |
| setActiveChainId(chainIdToUse); | |
| kit | |
| .transaction({ | |
| chainId: chainIdToUse, | |
| to: txData.to, | |
| value: txData.value, | |
| data: txData.data, | |
| }) | |
| .name({ transactionName: 'tx-payload-single-send' }); | |
| const chainIdToUse = payloadTx?.chainId ?? etherspotDefaultChainId; | |
| if (typeof chainIdToUse !== 'number') { | |
| handleError(t`error.invalidChainIdForTransaction`); | |
| return; | |
| } | |
| // Set the active chain context for this transaction | |
| // This ensures the transaction is built and sent on the correct chain | |
| setActiveChainId(chainIdToUse); | |
| kit | |
| .transaction({ | |
| chainId: chainIdToUse, | |
| to: txData.to, | |
| value: txData.value, | |
| data: txData.data, | |
| }) | |
| .name({ transactionName: 'tx-payload-single-send' }); |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 1259 to 1273, payloadTx?.chainId may be undefined which will break
setActiveChainId and kit.transaction; validate chainIdToUse before using it and
handle the undefined case—either (a) if chainId is required, throw or return
early with a clear error/UI message when undefined, or (b) otherwise derive a
safe default chainId (e.g., from current app context) and only call
setActiveChainId and kit.transaction with a defined chainId; ensure you do not
call setActiveChainId or kit.transaction with undefined and surface an
appropriate error or fallback to prevent runtime failures.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (6)
163-167: Use actual fee-asset symbol (not address) when formatting costsCurrently symbol = selectedFeeAsset.token?.toUpperCase(), which yields an address. Persist the symbol and use it in UI/logs.
Apply:
- const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ - token: string; - decimals: number; - tokenPrice?: string; - balance?: string; - }>(); + const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ + token: string; + decimals: number; + tokenPrice?: string; + balance?: string; + symbol?: string; + }>();setSelectedFeeAsset({ token: feeOptions[0].asset.contract, decimals: feeOptions[0].asset.decimals, tokenPrice: feeOptions[0].asset.price?.toString(), balance: feeOptions[0].value?.toString(), + symbol: feeOptions[0].asset.symbol, });setSelectedFeeAsset({ token: tokenAddress, decimals: Number(values[3]) ?? 18, tokenPrice: tokenOption.asset.price?.toString(), balance: tokenOption.value?.toString(), + symbol: tokenOption.asset.symbol, });- symbol = selectedFeeAsset.token?.toUpperCase() || ''; + symbol = selectedFeeAsset.symbol || 'ERC20';Also applies to: 313-319, 2133-2138, 543-546
976-1231: Payload batches flow is still deeply nested; extract helpers to reduce complexityThe build → estimate → validate → cost-check → send → finalize stages are all inline here. Previous comment suggested extracting helpers; doing so will significantly improve readability/testability.
1941-1964: Differentiate user-facing errors by type (rejection, insufficient funds, network)The catch block surfaces a generic message. Distinguishing known cases will improve UX and debuggability. Prior suggestion includes a ready diff.
591-797: Fix polling memory leak and stale state; set correct latest chain id; add cleanup
- Interval isn’t registered for cleanup → potential leaks on unmount.
- setLatestUserOpChainId should use the chain actually being polled (chainIdForTxHash), not selectedAsset?.chainId.
- Avoid reading userOpStatus from outer scope inside setInterval (stale closure on timeouts).
This has been raised previously; reiterating with concrete patch.
Apply:
- import React, { useEffect, useMemo } from 'react'; + import React, { useEffect, useMemo, useRef } from 'react';function startUserOpPolling({ @@ }) { + // Track active polling intervals for cleanup + const pollingIntervalsRef = (startUserOpPolling as any)._intervalsRef as React.MutableRefObject<number[]> + ?? (startUserOpPolling as any)._intervalsRef = { current: [] }; transactionDebugLog('Polling userOp status for hash:', userOpHash); @@ - setLatestUserOpChainId(selectedAsset?.chainId); + setLatestUserOpChainId(chainIdForTxHash); @@ - const userOperationStatus = setInterval(async () => { + const userOperationStatus = window.setInterval(async () => { attempts += 1; @@ - if (userOpStatus !== 'Confirmed') { - setUserOpStatus('Failed'); - // Sentry capturing ... - ... - } + setUserOpStatus('Failed'); + // Sentry capturing remains below } @@ }, userOpStatusInterval); + pollingIntervalsRef.current.push(userOperationStatus); } + + // Cleanup all polling intervals on unmount + useEffect(() => { + return () => { + const ref = (startUserOpPolling as any)._intervalsRef as React.MutableRefObject<number[]> | undefined; + if (ref?.current?.length) { + for (const id of ref.current) clearInterval(id); + ref.current = []; + } + }; + }, []);Note: For non-Error strings passed to Sentry, prefer captureMessage over captureException for clarity (optional).
Also applies to: 15-15, 614-616
1268-1273: Validate payload chainId before using; avoid undefined active chain contextPayload chainId may be undefined, which would break setActiveChainId and kit.transaction at runtime.
Apply:
- const chainIdToUse = payloadTx?.chainId; + const chainIdToUse = + typeof payloadTx?.chainId === 'number' + ? payloadTx.chainId + : etherspotDefaultChainId; + if (typeof chainIdToUse !== 'number') { + handleError(t`error.invalidChainIdForTransaction`); + return; + }
1492-1502: Guard approval transaction; skip when approveData is missingAdding an approval with empty data will fail estimation/send ambiguously. Only add approval if approveData is present.
Apply:
- // 1. Approval transaction - kit - .transaction({ - chainId: selectedAsset.chainId, - to: selectedFeeAsset.token, - value: '0', - data: approveData, - }) - .name({ transactionName: 'approve' }) - .addToBatch({ batchName }); + // 1. Approval transaction (only if we have computed approve data) + if (!approveData) { + handleError(t`error.approvalDataUnavailable`); + return; + } + kit + .transaction({ + chainId: selectedAsset.chainId, + to: selectedFeeAsset.token, + value: '0', + data: approveData, + }) + .name({ transactionName: 'approve' }) + .addToBatch({ batchName });
🧹 Nitpick comments (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
2055-2065: Avoid huge transaction names; use a short, stable identifierEmbedding the full data blob in transactionName can create very long keys and memory overhead. Prefer a shorter suffix.
Apply:
- const transactionName = `tx-${chainId}-${txData.data}`; + const shortData = + typeof txData.data === 'string' && txData.data.startsWith('0x') + ? `${txData.data.slice(0, 10)}` + : 'transfer'; + const transactionName = `tx-${chainId}-${shortData}-${Date.now()}`;
1215-1220: Sentry “send_success” logs use selectedAsset?.value instead of the actual amountselectedAsset?.value is the SelectOption’s value, not the user-entered amount. Log the actual amount (string) for accuracy.
Apply:
- amount: selectedAsset?.value, + amount,Repeat the same replacement in the other send_success contexts referenced above.
Also applies to: 1457-1471, 1714-1728, 1924-1938
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(23 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
PR: pillarwallet/x#320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
📚 Learning: 2025-08-20T09:14:16.864Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.864Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
🧬 Code graph analysis (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (8)
src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/services/tokensData.ts (1)
convertPortfolioAPIResponseToToken(97-121)src/utils/blockchain.ts (3)
getNativeAssetForChainId(69-141)safeBigIntConversion(358-366)buildTransactionData(368-474)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/utils/common.ts (1)
transactionDescription(62-75)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/types/blockchain.ts (1)
ITransaction(22-27)src/types/index.ts (2)
SelectOption(57-63)TokenAssetSelectOption(41-46)
🪛 Biome (2.1.2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
[error] 14-14: Do not shadow the global "isNaN" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (1)
2081-2082: Verify setActiveChainId signature
I wasn’t able to locate thesetActiveChainIddefinition in the codebase to confirm whether it acceptsundefined. Please check the context/provider or hook wheresetActiveChainIdis declared (e.g. in your active-chain context file) and ensure its parameter type includesundefined. If it only acceptsnumber, replace these calls with a valid fallback chain ID (for example, usingkit.getEtherspotProvider().getChainId()or the last-known chain ID) instead ofundefined:• SendModalTokensTabView.tsx, lines 2081–2082
• Also applies to lines 2121–2122
| import { isNaN } from 'lodash'; | ||
| import React, { useEffect, useMemo } from 'react'; |
There was a problem hiding this comment.
Remove lodash isNaN shadowing and harden payload value parsing
Biome flags the lodash isNaN import (shadows global). Also prefer a strict integer-string check when deciding if the payload value is already in wei.
Apply:
- import { isNaN } from 'lodash';
+ // removed lodash isNaN to avoid shadowing the global- // If it's already a numeric string without decimals, assume it's in wei
- if (!valueStr.includes('.') && !isNaN(Number(valueStr))) {
+ // If it's already an integer numeric string (no decimals), assume it's in wei
+ if (!valueStr.includes('.') && /^[0-9]+$/.test(valueStr)) {
return valueStr;
}Also applies to: 1245-1253
🧰 Tools
🪛 Biome (2.1.2)
[error] 14-14: Do not shadow the global "isNaN" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| const builtTxData = buildTransactionData({ | ||
| tokenAddress: selectedAsset.asset.contract, | ||
| recipient, | ||
| amount: valueToSend, | ||
| decimals: selectedAsset.asset.decimals, | ||
| }); | ||
| txData = { | ||
| ...builtTxData, | ||
| value: | ||
| typeof builtTxData.value === 'bigint' | ||
| ? builtTxData.value | ||
| : String(builtTxData.value ?? ''), | ||
| chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId), | ||
| }; |
There was a problem hiding this comment.
Wrap buildTransactionData in try/catch to handle validation errors gracefully
buildTransactionData throws on invalid recipient/amount/decimals. Without a try/catch, onAddToBatch can crash the component.
Apply:
- const builtTxData = buildTransactionData({
- tokenAddress: selectedAsset.asset.contract,
- recipient,
- amount: valueToSend,
- decimals: selectedAsset.asset.decimals,
- });
- txData = {
- ...builtTxData,
- value:
- typeof builtTxData.value === 'bigint'
- ? builtTxData.value
- : String(builtTxData.value ?? ''),
- chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId),
- };
+ try {
+ const builtTxData = buildTransactionData({
+ tokenAddress: selectedAsset.asset.contract,
+ recipient,
+ amount: valueToSend,
+ decimals: selectedAsset.asset.decimals,
+ });
+ txData = {
+ ...builtTxData,
+ value:
+ typeof builtTxData.value === 'bigint'
+ ? builtTxData.value
+ : String(builtTxData.value ?? ''),
+ chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId),
+ };
+ } catch (e) {
+ setErrorMessage(
+ e instanceof Error ? e.message : t`error.invalidTransactionData`
+ );
+ 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 builtTxData = buildTransactionData({ | |
| tokenAddress: selectedAsset.asset.contract, | |
| recipient, | |
| amount: valueToSend, | |
| decimals: selectedAsset.asset.decimals, | |
| }); | |
| txData = { | |
| ...builtTxData, | |
| value: | |
| typeof builtTxData.value === 'bigint' | |
| ? builtTxData.value | |
| : String(builtTxData.value ?? ''), | |
| chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId), | |
| }; | |
| try { | |
| const builtTxData = buildTransactionData({ | |
| tokenAddress: selectedAsset.asset.contract, | |
| recipient, | |
| amount: valueToSend, | |
| decimals: selectedAsset.asset.decimals, | |
| }); | |
| txData = { | |
| ...builtTxData, | |
| value: | |
| typeof builtTxData.value === 'bigint' | |
| ? builtTxData.value | |
| : String(builtTxData.value ?? ''), | |
| chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId), | |
| }; | |
| } catch (e) { | |
| setErrorMessage( | |
| e instanceof Error ? e.message : t`error.invalidTransactionData` | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx around
lines 2003 to 2016, buildTransactionData can throw on invalid
recipient/amount/decimals so calling it directly may crash onAddToBatch; wrap
the buildTransactionData call and the subsequent txData assignment in a
try/catch, catch the validation error, log or surface it (e.g. set a
form/validation error or toast), ensure txData is left unset/cleared, and exit
the add-to-batch flow early so the component does not crash or add an invalid
transaction.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
src/providers/EtherspotTransactionKitProvider.tsx (2)
36-41: Avoid re-instantiating the kit on every config change; keep a stable instance and update in placeRecreating
EtherspotTransactionKitwheneverconfigchanges can drop in-flight state (named transactions, batches) and cause UX glitches. Prefer a single instance stored in a ref and apply config/chain updates via an updater API if available. Also,kitRefis currently only used to mirror the new instance; either leverage it for stability or remove it.If
EtherspotTransactionKitexposessetChainIdorgetEtherspotProvider().updateConfig, keep a single instance and update in-place:- const kit = useMemo(() => { - const newKit = new EtherspotTransactionKit(config); - kitRef.current = newKit; - return newKit; - }, [config]); + // Create once + const kit = useMemo(() => { + const newKit = new EtherspotTransactionKit(config); + kitRef.current = newKit; + return newKit; + }, []); + + // Update config/chain on change without recreating + useEffect(() => { + const k = kitRef.current as EtherspotTransactionKit | null; + if (!k) return; + const provider = k.getEtherspotProvider?.(); + try { + // Prefer an explicit method if available + provider?.updateConfig?.(config); + // or k.setChainId?.(config.chainId); + } catch (e) { + console.warn('EtherspotTransactionKit: failed to update config', e); + } + }, [config]);To ensure no code depends on re-instantiation semantics, run:
#!/bin/bash # Verify whether setChainId or updateConfig exist and where kit recreation is assumed rg -nP "setChainId\s*\(|updateConfig\s*\(|new\s+EtherspotTransactionKit\s*\(" --type=ts --type=tsx -C3
59-66: Re-enable jsx-no-constructed-context-values; memoize the Provider value wrapperYou already memoize
contextData, but{ data: contextData }is constructed inline on every render. Memoize the wrapper to avoid needless downstream re-renders and remove the ESLint disable.-/* eslint-disable react/jsx-no-constructed-context-values */ @@ const contextData = useMemo( () => ({ walletAddress, setWalletAddress, kit, }), [walletAddress, kit] ); + const contextValue = useMemo(() => ({ data: contextData }), [contextData]); @@ - return ( - <EtherspotTransactionKitContext.Provider value={{ data: contextData }}> + return ( + <EtherspotTransactionKitContext.Provider value={contextValue}> {children} </EtherspotTransactionKitContext.Provider> );Also applies to: 69-72
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (7)
162-167: Show the actual gas token symbol for paymaster mode (not the token address)
symbol = selectedFeeAsset.token?.toUpperCase()yields an address. Carry the symbol alongside the address and display it.- const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ - token: string; - decimals: number; - tokenPrice?: string; - balance?: string; - }>(); + const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ + token: string; + decimals: number; + tokenPrice?: string; + balance?: string; + symbol?: string; + }>(); @@ setSelectedFeeAsset({ token: feeOptions[0].asset.contract, decimals: feeOptions[0].asset.decimals, tokenPrice: feeOptions[0].asset.price?.toString(), balance: feeOptions[0].value?.toString(), + symbol: feeOptions[0].asset.symbol, }); @@ - symbol = selectedFeeAsset.token?.toUpperCase() || ''; + symbol = selectedFeeAsset.symbol || 'ERC20'; decimals = selectedFeeAsset.decimals ?? 18; price = Number(selectedFeeAsset.tokenPrice) || 0;Also set
symbolinhandleOnChangewhen switching fee assets.Also applies to: 313-319, 543-556
1934-1957: Differentiate user-facing errors; provide specific guidanceAll failures map to the same generic message. Tailor messages for rejections, network issues, and insufficient funds to improve UX and triage.
- } catch (error: unknown) { - Sentry.captureException(error, { + } catch (error: unknown) { + let errorMessage = t`error.genericSendFailure`; + let errorLevel: 'error' | 'warning' = 'error'; + if (error instanceof Error) { + const msg = error.message.toLowerCase(); + if (msg.includes('reject') || msg.includes('denied')) { + errorMessage = t`error.userRejectedTransaction`; + errorLevel = 'warning'; + } else if (msg.includes('insufficient funds')) { + errorMessage = t`error.insufficientFunds`; + } else if (msg.includes('network') || msg.includes('timeout')) { + errorMessage = t`error.networkError`; + } + } + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'send_error', sendId, }, contexts: { send_error: { sendId, error: error instanceof Error ? error.message : String(error), selectedAsset: getAssetSymbol(selectedAsset), amount, recipient, }, }, }); - - handleError( - 'Something went wrong while sending the assets, please try again later. If the problem persists, contact the PillarX team for support.' - ); + handleError(errorMessage);
14-15: Fix lint error and harden numeric checks; remove lodash isNaN shadowingImporting
isNaNfrom lodash shadows the global and fails Biome. Also, the decimal/wei heuristic should use a strict integer-string test.-import { isNaN } from 'lodash'; -import React, { useEffect, useMemo } from 'react'; +import React, { useEffect, useMemo, useRef } from 'react'; // add useRef for polling cleanup @@ - // If it's already a numeric string without decimals, assume it's in wei - if (!valueStr.includes('.') && !isNaN(Number(valueStr))) { + // If it's already an integer numeric string (no decimals), assume it's in wei + if (!valueStr.includes('.') && /^[0-9]+$/.test(valueStr)) { return valueStr; }Also applies to: 1248-1253
591-797: Fix polling memory leak, stale closure, and wrong “latest chain” association
- The interval isn’t cleaned up on unmount → leak.
setLatestUserOpChainId(selectedAsset?.chainId)should use the actual queried chain (chainIdForTxHash).- Avoid relying on outer
userOpStatusinside the interval (stale closure).-/* eslint-disable no-plusplus */ -/* eslint-disable @typescript-eslint/no-use-before-define */ -import React, { useEffect, useMemo } from 'react'; +/* eslint-disable no-plusplus */ +/* eslint-disable @typescript-eslint/no-use-before-define */ +import React, { useEffect, useMemo, useRef } from 'react'; @@ + // Track active polling intervals for cleanup + const pollingIntervalsRef = useRef<number[]>([]); @@ - setLatestUserOpChainId(selectedAsset?.chainId); + setLatestUserOpChainId(chainIdForTxHash); @@ - const userOperationStatus = setInterval(async () => { + const userOperationStatus = window.setInterval(async () => { attempts += 1; @@ - if (attempts >= maxAttempts) { - clearInterval(userOperationStatus); + if (attempts >= maxAttempts) { + clearInterval(userOperationStatus); transactionDebugLog( 'Max attempts reached without userOp with OnChain status. Check transaction hash:', response?.transaction ); - if (userOpStatus !== 'Confirmed') { - setUserOpStatus('Failed'); - // Sentry capturing ... - ... - } + setUserOpStatus('Failed'); + setTransactionHash(response?.transaction); } @@ - clearInterval(userOperationStatus); + clearInterval(userOperationStatus); setUserOpStatus('Failed'); @@ - }, userOpStatusInterval); + }, userOpStatusInterval); + pollingIntervalsRef.current.push(userOperationStatus); } + + // Cleanup polling intervals on unmount + useEffect(() => { + return () => { + for (const id of pollingIntervalsRef.current) clearInterval(id); + pollingIntervalsRef.current = []; + }; + }, []);Also applies to: 15-15, 614-616, 742-777
1268-1276: Validate/fallback chainId in payload single-transaction flow
chainIdToUsecan be undefined, causing runtime errors insidekit.transaction.- const chainIdToUse = payloadTx?.chainId; + const chainIdToUse = payloadTx?.chainId ?? etherspotDefaultChainId; + if (typeof chainIdToUse !== 'number') { + handleError(t`error.invalidChainIdForTransaction`); + return; + } @@ - .transaction({ - chainId: chainIdToUse, + .transaction({ + chainId: chainIdToUse,
1489-1497: Skip approval transaction when approveData is emptyAdding an empty-data approval leads to ambiguous failures.
- // 1. Approval transaction - kit - .transaction({ - chainId: selectedAsset.chainId, - to: selectedFeeAsset.token, - value: '0', - data: approveData, - }) - .name({ transactionName: 'approve' }) - .addToBatch({ batchName }); + // 1. Approval transaction (only if precomputed) + if (!approveData) { + handleError(t`error.approvalDataUnavailable`); + return; + } + kit + .transaction({ + chainId: selectedAsset.chainId, + to: selectedFeeAsset.token, + value: '0', + data: approveData, + }) + .name({ transactionName: 'approve' }) + .addToBatch({ batchName });
1984-2011: Wrap buildTransactionData in try/catch to prevent crashes during batch-add
buildTransactionDatathrows on invalid input. Catch and present a validation error instead of crashing the flow.- const builtTxData = buildTransactionData({ - tokenAddress: selectedAsset.asset.contract, - recipient, - amount: valueToSend, - decimals: selectedAsset.asset.decimals, - }); - txData = { - ...builtTxData, - value: - typeof builtTxData.value === 'bigint' - ? builtTxData.value - : String(builtTxData.value ?? ''), - chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId), - }; + try { + const builtTxData = buildTransactionData({ + tokenAddress: selectedAsset.asset.contract, + recipient, + amount: valueToSend, + decimals: selectedAsset.asset.decimals, + }); + txData = { + ...builtTxData, + value: + typeof builtTxData.value === 'bigint' + ? builtTxData.value + : String(builtTxData.value ?? ''), + chainId: Number(selectedAsset.chainId ?? etherspotDefaultChainId), + }; + } catch (e) { + setErrorMessage( + e instanceof Error ? e.message : t`error.invalidTransactionData` + ); + return; + }__mocks__/useTransactionKit.ts (1)
3-27: Add EIP-1193 provider stub to prevent test failuresWe’ve confirmed that
src/apps/the-exchange/index.tsxdirectly callskit.getProvider()(at line 32), so without mocking it the tests will crash whengetProvider()is undefined. Please update__mocks__/useTransactionKit.tsto include a minimal provider stub:const useTransactionKit = vi.fn(() => ({ kit: { + getProvider: vi.fn(() => ({ + // Minimal EIP-1193-like provider for .request() and event listeners + request: vi.fn(async () => null), + on: vi.fn(), + removeListener: vi.fn(), + off: vi.fn(), + })), getState: vi.fn(() => ({ namedTransactions: {}, batches: {}, isEstimating: false, isSending: false, containsSendingError: false, containsEstimatingError: false, })), getEtherspotProvider: vi.fn(() => ({ getChainId: vi.fn(() => 1), })), transaction: vi.fn(() => ({ name: vi.fn(() => ({ estimate: vi.fn(() => Promise.resolve({})), send: vi.fn(() => Promise.resolve({})), })), })), estimateBatches: vi.fn(() => Promise.resolve({})), sendBatches: vi.fn(() => Promise.resolve({})), }, walletAddress: '0x1234567890123456789012345678901234567890', setWalletAddress: vi.fn(), }));This ensures any
.request()or event-listener calls on the provider won’t throw.
🧹 Nitpick comments (13)
src/providers/EtherspotTransactionKitProvider.tsx (2)
44-57: Harden wallet address fetch against race conditionsIf
configchanges quickly (or the component unmounts),setWalletAddressmay run on a stale/unmounted component. Add a mounted flag or an AbortController to avoid state updates after unmount.useEffect(() => { - const getWalletAddress = async () => { + let mounted = true; + const getWalletAddress = async () => { if (kit) { try { const address = await kit.getWalletAddress(); - setWalletAddress(address); + if (mounted) setWalletAddress(address); } catch (error) { console.error('Failed to get wallet address:', error); } } }; getWalletAddress(); - }, [kit]); + return () => { + mounted = false; + }; + }, [kit]);
34-41: Remove unused kitRef or put it to use
kitRefis only written but never read. Either remove it or use it for the in-place config update pattern proposed above.- const kitRef = useRef<EtherspotTransactionKit | null>(null); + // Remove if not used for in-place updates__mocks__/EtherspotTransactionKitProvider.tsx (3)
14-24: Defaulting the mock Context to non-null can mask missing Provider usageSetting a non-null default value allows consumers to “work” without a Provider, diverging from production where the hook should throw if context is null. Prefer default
nulland have the mock Provider supply the value.-export const EtherspotTransactionKitContext = - createContext<EtherspotTransactionKitContextType | null>({ - data: { - kit: { - // Mock kit methods, can add more as needed for the tests - getWalletAddress: () => Promise.resolve('0xMockWalletAddress'), - }, - walletAddress: '0xMockWalletAddress', - setWalletAddress: () => {}, - }, - }); +export const EtherspotTransactionKitContext = + createContext<EtherspotTransactionKitContextType | null>(null);Then keep the concrete value inside the mock
Provider.
26-30: Remove unused prop ‘config’ from the mock Provider
configis declared but unused. Drop it to avoid confusion.-export const EtherspotTransactionKitProvider: React.FC<{ - // eslint-disable-next-line @typescript-eslint/no-explicit-any, react/no-unused-prop-types - config: any; - children: React.ReactNode; -}> = ({ children }) => { +export const EtherspotTransactionKitProvider: React.FC<{ + children: React.ReactNode; +}> = ({ children }) => {
31-45: (Optional) Memoize the constructed context value to reduce re-rendersEven in tests, memoizing the object identity helps when components rely on referential equality.
- return ( - <EtherspotTransactionKitContext.Provider - value={{ - data: { - kit: { - getWalletAddress: () => Promise.resolve('0xMockWalletAddress'), - }, - walletAddress: '0xMockWalletAddress', - setWalletAddress: () => {}, - }, - }} - > + const value = React.useMemo( + () => ({ + data: { + kit: { getWalletAddress: () => Promise.resolve('0xMockWalletAddress') }, + walletAddress: '0xMockWalletAddress', + setWalletAddress: () => {}, + }, + }), + [] + ); + return ( + <EtherspotTransactionKitContext.Provider value={value}> {children} </EtherspotTransactionKitContext.Provider> );src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
131-146: Avoid calling useTransactionKit twice; destructure onceCall the hook once to avoid duplicate context reads and ensure both values remain in sync.
- const { kit } = useTransactionKit(); + const { kit, walletAddress: accountAddress } = useTransactionKit(); @@ - const { walletAddress: accountAddress } = useTransactionKit();
2050-2058: Avoid huge transaction names; use a stable, short identifierUsing the entire
datafield in the transaction name can bloat memory and logs. Hash or slice it.- const transactionName = `tx-${chainId}-${txData.data}`; + const dataSnippet = (txData.data || '').slice(0, 16); + const transactionName = `tx-${chainId}-${dataSnippet}`;src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx (1)
12-13: Good migration to useTransactionKit mock; consider using vi.mocked for typesThe tests correctly mock
useTransactionKitand drive address state. For cleaner typing, considervi.mocked()instead ofas unknown as Mock.Example:
const useTransactionKitMock = vi.mocked(useTransactionKit); useTransactionKitMock.mockReturnValue({ walletAddress: '0x123', kit: {} });Also applies to: 27-34, 37-44
__mocks__/useTransactionKit.ts (1)
13-24: Consider returning realistic shapes from estimate/send helpers to reduce test stubbing churnWhere feasible, mirror kit v2 outputs (e.g.,
{ isEstimatedSuccessfully, cost, chainId }and{ isSentSuccessfully, userOpHash, chainId }). This prevents breaking tests when code starts reading those fields.- transaction: vi.fn(() => ({ - name: vi.fn(() => ({ - estimate: vi.fn(() => Promise.resolve({})), - send: vi.fn(() => Promise.resolve({})), - })), - })), - estimateBatches: vi.fn(() => Promise.resolve({})), - sendBatches: vi.fn(() => Promise.resolve({})), + transaction: vi.fn(() => ({ + name: vi.fn(() => ({ + estimate: vi.fn(() => Promise.resolve({ isEstimatedSuccessfully: true, cost: 0n, chainId: 1 })), + send: vi.fn(() => Promise.resolve({ isSentSuccessfully: true, userOpHash: '0xUSEROP', chainId: 1 })), + })), + })), + estimateBatches: vi.fn(() => + Promise.resolve({ isEstimatedSuccessfully: true, batches: {} }) + ), + sendBatches: vi.fn(() => + Promise.resolve({ isSentSuccessfully: true, batches: {} }) + ),src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx (4)
35-41: Use a minimal provider stub or rely entirely on the mocked kitSince the provider constructs the mocked kit, the provider field here isn’t used. If you keep it, stub a realistic shape to avoid accidental accesses later.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - provider: {} as any, + // Minimal EIP-1193-ish stub (safe if code ever touches it) + provider: { request: vi.fn() } as unknown as Record<string, unknown>,
1-4: Add user-event and clear mocks between testsUse @testing-library/user-event for realistic interactions and clear mocks to prevent leakage across tests.
-import { render, screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; @@ -import { describe, expect, it, vi } from 'vitest'; +import { describe, expect, it, vi, afterEach } from 'vitest'; + +afterEach(() => { + vi.clearAllMocks(); +});
21-33: Trim unused mock surface to reduce noisetransaction, estimateBatches, and sendBatches aren’t used by these tests. Removing them will simplify the mock and make intent clearer.
getWalletAddress: vi.fn(() => Promise.resolve('0xMockWalletAddress')), - getEtherspotProvider: vi.fn(() => ({ - getChainId: vi.fn(() => 1), - })), - transaction: vi.fn(() => ({ - name: vi.fn(() => ({ - estimate: vi.fn(() => Promise.resolve({})), - send: vi.fn(() => Promise.resolve({})), - })), - })), - estimateBatches: vi.fn(() => Promise.resolve({})), - sendBatches: vi.fn(() => Promise.resolve({})), + getEtherspotProvider: vi.fn(() => ({ getChainId: vi.fn(() => 1) })),
54-77: Optional: add an error-path test for robustnessProvider logs and leaves walletAddress undefined when kit.getWalletAddress rejects. Add a test to lock that behavior in.
Example (no diff, new test block):
it('handles getWalletAddress failure gracefully', async () => { const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); // Force the mock to reject for this test (mockKit.getWalletAddress as jest.Mock | vi.Mock).mockRejectedValueOnce(new Error('boom')); const TestComponent = () => { const ctx = React.useContext(EtherspotTransactionKitContext); if (!ctx) return null; return <div data-testid="wallet-address">{ctx.data.walletAddress ?? 'No address'}</div>; }; render( <EtherspotTransactionKitProvider config={mockConfig}> <TestComponent /> </EtherspotTransactionKitProvider> ); expect(await screen.findByTestId('wallet-address')).toHaveTextContent('No address'); expect(consoleSpy).toHaveBeenCalled(); consoleSpy.mockRestore(); });I can push a follow-up commit adding this negative-path test if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/apps/pillarx-app/components/PortfolioOverview/test/__snapshots__/PortfolioOverview.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
__mocks__/EtherspotTransactionKitProvider.tsx(1 hunks)__mocks__/useTransactionKit.ts(1 hunks)package.json(1 hunks)src/apps/leaderboard/components/LeaderboardTab/test/LeaderboardTab.test.tsx(6 hunks)src/apps/leaderboard/components/PxPointsSummary/tests/PxPointsSummary.test.tsx(4 hunks)src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx(9 hunks)src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx(4 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(23 hunks)src/providers/EtherspotTransactionKitProvider.tsx(1 hunks)src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/apps/pillarx-app/components/WalletPortfolioTile/test/WalletPortfolioTile.test.tsx
- package.json
- src/apps/leaderboard/components/PxPointsSummary/tests/PxPointsSummary.test.tsx
- src/apps/leaderboard/components/LeaderboardTab/test/LeaderboardTab.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
__mocks__/useTransactionKit.tssrc/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsxsrc/providers/__tests__/EtherspotTransactionKitProvider.test.tsxsrc/providers/EtherspotTransactionKitProvider.tsxsrc/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx__mocks__/EtherspotTransactionKitProvider.tsx
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
PR: pillarwallet/x#320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
🧬 Code graph analysis (4)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (8)
src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/services/tokensData.ts (1)
convertPortfolioAPIResponseToToken(97-121)src/utils/blockchain.ts (3)
getNativeAssetForChainId(69-141)safeBigIntConversion(358-366)buildTransactionData(368-474)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/utils/common.ts (1)
transactionDescription(62-75)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/types/blockchain.ts (1)
ITransaction(22-27)src/types/index.ts (2)
SelectOption(57-63)TokenAssetSelectOption(41-46)
src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx (2)
__mocks__/EtherspotTransactionKitProvider.tsx (2)
EtherspotTransactionKitProvider(26-46)EtherspotTransactionKitContext(14-24)src/providers/EtherspotTransactionKitProvider.tsx (2)
EtherspotTransactionKitProvider(30-73)EtherspotTransactionKitContext(22-23)
src/providers/EtherspotTransactionKitProvider.tsx (1)
__mocks__/EtherspotTransactionKitProvider.tsx (3)
EtherspotTransactionKitContextType(5-12)EtherspotTransactionKitContext(14-24)EtherspotTransactionKitProvider(26-46)
__mocks__/EtherspotTransactionKitProvider.tsx (1)
src/providers/EtherspotTransactionKitProvider.tsx (3)
EtherspotTransactionKitContextType(14-20)EtherspotTransactionKitContext(22-23)EtherspotTransactionKitProvider(30-73)
🪛 Biome (2.1.2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
[error] 14-14: Do not shadow the global "isNaN" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (3)
src/providers/EtherspotTransactionKitProvider.tsx (1)
61-64: RetainsetWalletAddressin the contextThe project’s own tests in
src/providers/__tests__/EtherspotTransactionKitProvider.test.tsxexplicitly assert that:
context.data.setWalletAddressexists (lines 71, 118)- It can be called by an external consumer (lines 101–103)
Removing the setter from the public context would break these tests and violate the established public contract.
Key locations:
- Provider implementation (
src/providers/EtherspotTransactionKitProvider.tsx)
- Declaration:
const [walletAddress, setWalletAddress] = useState<string>();(line 33)- Internal updates:
setWalletAddress(address);(line 49)- Memoized context: includes
setWalletAddress(lines 61–64)- Test suite (
src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx)
- Existence check:
typeof context.data.setWalletAddress === 'function'(lines 71, 118)- Invocation test: calls
context.data.setWalletAddress(...)(lines 101–103)Because these tests define and verify the public API surface, the suggestion to remove
setWalletAddressis not applicable.Likely an incorrect or invalid review comment.
src/apps/pillarx-app/components/ReceiveModal/test/ReceiveModal.test.tsx (1)
94-118: Nice coverage of close interactions (ESC button/key and outside click)These interactions are well covered and assertions verify the reducer dispatch. No changes needed.
Also applies to: 135-157, 159-184
src/providers/__tests__/EtherspotTransactionKitProvider.test.tsx (1)
43-53: LGTM: child rendering sanity checkThis test is sound as a basic mount smoke test.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (9)
571-585: Show correct fee asset symbol; avoid rendering ERC20 address as “symbol”Persist the fee-asset symbol when selecting it and use that for cost display instead of
selectedFeeAsset.token?.toUpperCase()(an address).- symbol = selectedFeeAsset.token?.toUpperCase() || ''; + symbol = selectedFeeAsset.symbol || 'ERC20';Add symbol to the state shape:
- const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ - token: string; - decimals: number; - tokenPrice?: string; - balance?: string; - }>(); + const [selectedFeeAsset, setSelectedFeeAsset] = React.useState<{ + token: string; + decimals: number; + tokenPrice?: string; + balance?: string; + symbol?: string; + }>();Persist symbol when auto-selecting first option:
setSelectedFeeAsset({ token: firstOption.asset.contract, decimals: firstOption.asset.decimals, tokenPrice: firstOption.asset.price?.toString(), balance: firstOption.value?.toString(), + symbol: firstOption.asset.symbol, });Persist symbol on fee asset change:
setSelectedFeeAsset({ token: tokenAddress, decimals: Number(values[3]) ?? 18, tokenPrice: tokenOption.asset.price?.toString(), balance: tokenOption.value?.toString(), + symbol: tokenOption.asset.symbol, });Also applies to: 162-167, 330-335, 2151-2156
1780-1808: Improve error reporting with specific user-facing messagesDifferentiate between rejection, insufficient funds, and network issues for better UX and triage.
- if (estimated.errorMessage) { + if (estimated.errorMessage) { ... - handleError( - 'Something went wrong while estimating the asset transfer. Please try again later. If the problem persists, contact the PillarX team for support.' - ); + const msg = estimated.errorMessage.toLowerCase().includes('insufficient') + ? t`error.insufficientFunds` + : t`error.estimationFailed`; + handleError(msg); return; }- } catch (error: unknown) { - Sentry.captureException(error, { + } catch (error: unknown) { + let errorMessage = t`error.genericSendFailure`; + if (error instanceof Error) { + const msg = error.message.toLowerCase(); + if (msg.includes('rejected') || msg.includes('denied')) { + errorMessage = t`error.userRejectedTransaction`; + } else if (msg.includes('insufficient funds')) { + errorMessage = t`error.insufficientFunds`; + } else if (msg.includes('network') || msg.includes('timeout')) { + errorMessage = t`error.networkError`; + } + } + Sentry.captureException(error, { tags: { component: 'send_flow', action: 'send_error', sendId }, contexts: { send_error: { sendId, error: error instanceof Error ? error.message : String(error), selectedAsset: getAssetSymbol(selectedAsset), amount, recipient, }, }, }); - - handleError( - 'Something went wrong while sending the assets, please try again later. If the problem persists, contact the PillarX team for support.' - ); + handleError(errorMessage);Also applies to: 1962-1983
14-15: Remove lodash isNaN (lint error) and add useRef for interval management
- Biome flags the lodash isNaN import as shadowing the global. Replace it with a strict integer-string check.
- You also create intervals without registering a component-unmount cleanup. Add useRef now (used below).
- import { isNaN } from 'lodash'; -import React, { useEffect, useMemo } from 'react'; + // removed lodash isNaN to avoid shadowing the global +import React, { useEffect, useMemo, useRef } from 'react';
634-643: Use the actual queried chain for latestUserOpChainId
setLatestUserOpChainId(selectedAsset?.chainId)can be wrong for payload and cross-chain flows; use the chain you’re polling on.- setLatestUserOpChainId(selectedAsset?.chainId); + setLatestUserOpChainId(chainIdForTxHash);
660-825: Fix polling leak and stale state; register unmount cleanup
- The interval is not guaranteed to be cleared on component unmount → potential leaks.
- Avoid relying on outer-scope state inside the interval; you already compute status from RPC, so the
userOpStatus !== 'Confirmed'guard is stale.- const userOperationStatus = setInterval(async () => { + const userOperationStatus = window.setInterval(async () => { attempts += 1; @@ - if (userOpStatus !== 'Confirmed') { - setUserOpStatus('Failed'); - // Sentry capturing ... - setTransactionHash(response?.transaction); - } + setUserOpStatus('Failed'); + setTransactionHash(response?.transaction); } @@ - }, userOpStatusInterval); + }, userOpStatusInterval); + // track for unmount cleanup + pollingIntervalsRef.current.push(userOperationStatus);Additional code to add (outside this range):
// 1) near other hooks (after line 193 is fine) const pollingIntervalsRef = useRef<number[]>([]); // 2) once in a top-level effect for unmount cleanup useEffect(() => { return () => { for (const id of pollingIntervalsRef.current) clearInterval(id); pollingIntervalsRef.current = []; }; }, []);
1272-1289: Harden payload value parsing and guard undefined chainId
- Use a strict integer-string check for “already wei” instead of Number/isNaN.
- Ensure a defined chainId, with a safe fallback and type check.
- // If it's already a numeric string without decimals, assume it's in wei - if (!valueStr.includes('.') && !isNaN(Number(valueStr))) { + // If it's already an integer numeric string (no decimals), assume it's in wei + if (!valueStr.includes('.') && /^[0-9]+$/.test(valueStr)) { return valueStr; }- const chainIdToUse = payloadTx?.chainId; + const chainIdToUse = payloadTx?.chainId ?? etherspotDefaultChainId; + if (typeof chainIdToUse !== 'number') { + handleError(t`error.invalidChainIdForTransaction`); + return; + }Also applies to: 1296-1301
1012-1023: Validate each payload-batch transaction recipient before queuingAdd a guard so invalid/missing
tx.tois caught early instead of failing in the SDK.for (let txIdx = 0; txIdx < batch.transactions.length; txIdx++) { const tx = batch.transactions[txIdx]; + if (!tx?.to || !isAddress(tx.to)) { + transactionDebugLog(`Invalid recipient in batch ${batch.chainId} at index ${txIdx}`, tx); + Sentry.captureMessage('Invalid batch transaction recipient', { + level: 'error', + contexts: { invalid_tx: { batchIdx, txIdx, tx } }, + }); + handleError(t`error.invalidRecipientAddress`); + return; + } kit .transaction({ chainId: batch.chainId, to: tx.to,
1516-1525: Skip approval when approveData is emptyAdding an empty-data approval will fail estimation/send ambiguously.
- // 1. Approval transaction - kit - .transaction({ - chainId: selectedAsset.chainId, - to: selectedFeeAsset.token, - value: '0', - data: approveData, - }) - .name({ transactionName: 'approve' }) - .addToBatch({ batchName }); + // 1. Approval transaction (only if available) + if (!approveData) { + handleError(t`error.approvalDataUnavailable`); + return; + } + kit + .transaction({ + chainId: selectedAsset.chainId, + to: selectedFeeAsset.token, + value: '0', + data: approveData, + }) + .name({ transactionName: 'approve' }) + .addToBatch({ batchName });
2024-2037: Wrap buildTransactionData in try/catch to avoid crashing on invalid inputsThe helper throws on invalid recipient/amount/decimals.
- const builtTxData = buildTransactionData({ - tokenAddress: selectedAsset.asset.contract, - recipient, - amount: valueToSend, - decimals: selectedAsset.asset.decimals, - }); + let builtTxData; + try { + builtTxData = buildTransactionData({ + tokenAddress: selectedAsset.asset.contract, + recipient, + amount: valueToSend, + decimals: selectedAsset.asset.decimals, + }); + } catch (e) { + setErrorMessage(e instanceof Error ? e.message : t`error.invalidTransactionData`); + return; + }src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (4)
436-465: Remove stale outer-state guard in timeout pathUsing
userOpStatus !== 'Confirmed'inside the interval is prone to stale reads. You already decide based on RPC status; set Failed deterministically on timeout.- if (userOpStatus !== 'Confirmed') { - setUserOpStatus('Failed'); - // Sentry capturing - ... - } + setUserOpStatus('Failed'); + // Sentry capturing + ...
508-529: Use the same sendId in the catch block for coherent Sentry tracesA new random sendId breaks correlation with preceding breadcrumbs.
- } catch (error) { - Sentry.captureException(error, { + } catch (error) { + Sentry.captureException(error, { tags: { component: 'send_flow', - action: 'batch_send_error', - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + action: 'batch_send_error', + sendId, }, contexts: { - batch_send_error: { - sendId: `send_batch_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`, + batch_send_error: { + sendId, chainId, batchName, error: error instanceof Error ? error.message : String(error), }, }, });
616-619: Scope removal to the target batch to avoid deleting the wrong transactionCalling
kit.name(...).remove()without scoping can remove a same-named transaction in another batch.- kit.name({ transactionName }); // select the transaction - kit.remove(); // remove the selected transaction + // Select by batch then by transaction name, then remove + kit.batch({ batchName }).name({ transactionName }).remove(); triggerUpdate();
338-485: Register unmount cleanup for the polling intervalAs in the tokens tab, ensure the interval is cleaned up on unmount to prevent leaks.
- const userOperationStatus = setInterval(async () => { + const userOperationStatus = window.setInterval(async () => { attempts += 1; ... - }, userOpStatusInterval); + }, userOpStatusInterval); + pollingIntervalsRef.current.push(userOperationStatus);Additional code to add (outside this range):
// top: import useRef // import React, { useRef } from 'react'; // after hooks: const pollingIntervalsRef = React.useRef<number[]>([]); // unmount cleanup: React.useEffect(() => { return () => { for (const id of pollingIntervalsRef.current) clearInterval(id); pollingIntervalsRef.current = []; }; }, []);
🧹 Nitpick comments (4)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (2)
2076-2086: Avoid using raw data as part of transactionNameHex-encoded data can produce excessively long names and collisions. Prefer a concise, deterministic name.
- const transactionName = `tx-${chainId}-${txData.data}`; + const transactionName = `tx-${chainId}-${Date.now().toString(36)}`;
950-993: Do not silently abort on missing feeMin; surface errorReturning early without a message when
!feeMinmakes the UX opaque.- if (!feeMin) return; + if (!feeMin) { + handleError(t`error.unableToEstimateGasTokenAmount`); + return; + }src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (2)
194-207: Use consistent decimals for cost formattingEarlier you used the native asset decimals; at success logging you switch to fixed 18. Prefer consistency.
- const estimatedCost = ethers.utils.formatUnits( - estimatedCostBN, - nativeAsset.decimals - ); + const estimatedCost = ethers.utils.formatUnits( + estimatedCostBN, + nativeAsset.decimals + );- estimatedCost: estimatedCostBN - ? ethers.utils.formatUnits(estimatedCostBN, 18) + estimatedCost: estimatedCostBN + ? ethers.utils.formatUnits(estimatedCostBN, getNativeAssetForChainId(chainId).decimals) : null,Also applies to: 488-504
69-88: Consider subscribing to kit state changes to avoid manual forceUpdateReading
kit.getState().batchesonce per render means you must calltriggerUpdate()after every external mutation. If TxKit exposes a subscribe or event API, wiring that would keep UI in sync without manual forcing.Would you like me to check the SDK docs and propose a small wrapper hook (e.g., useKitBatches) that subscribes and memoizes grouped batches?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx(14 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(23 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
PR: pillarwallet/x#351
File: src/apps/pulse/utils/intent.ts:44-53
Timestamp: 2025-08-12T07:42:24.656Z
Learning: In the Pulse app's intent utilities (src/apps/pulse/utils/intent.ts), the team has chosen to use floating-point arithmetic for token amount calculations despite potential precision issues, accepting JavaScript's decimal place limitations as a valid trade-off for their use case.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
📚 Learning: 2025-05-28T14:30:02.702Z
Learnt from: RanaBug
PR: pillarwallet/x#320
File: src/components/BottomMenuModal/HistoryModal/TransactionInfo.tsx:177-182
Timestamp: 2025-05-28T14:30:02.702Z
Learning: In the transaction history tracking feature, chain ID validation is performed earlier in the flow before the values reach the TransactionInfo component, so additional validation at the display level is not needed.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
PR: pillarwallet/x#374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsxsrc/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx
🧬 Code graph analysis (2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (8)
src/hooks/useTransactionDebugLogger.tsx (1)
useTransactionDebugLogger(1-15)src/services/tokensData.ts (1)
convertPortfolioAPIResponseToToken(97-121)src/services/gasless.ts (1)
getAllGaslessPaymasters(23-56)src/utils/blockchain.ts (3)
getNativeAssetForChainId(69-141)safeBigIntConversion(358-366)buildTransactionData(368-474)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/utils/common.ts (1)
transactionDescription(62-75)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/types/blockchain.ts (1)
ITransaction(22-27)
src/components/BottomMenuModal/SendModal/SendModalBatchesTabView.tsx (3)
src/utils/blockchain.ts (2)
getChainName(260-279)getNativeAssetForChainId(69-141)src/utils/number.tsx (1)
formatAmountDisplay(3-33)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)
🪛 Biome (2.1.2)
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx
[error] 14-14: Do not shadow the global "isNaN" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Changes
Chores
Tests
Documentation