Conversation
WalkthroughAdds a new Key Wallet app under src/apps/key-wallet: UI components, Tailwind styling/config, TypeScript types, blockchain & portfolio utilities, media helpers, provider context exposure, manifest/README, and comprehensive tests; implements portfolio fetching, asset transforms, send flows (chain switching, ERC‑20/native send), and transaction tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Key Wallet App
participant API as Portfolio API
participant Modal as SendAssetModal
participant Blockchain as Blockchain Utils
participant Provider as Wallet Provider
User->>App: Open app / load wallet
App->>API: Fetch portfolio for EOA
API-->>App: Portfolio data
App->>App: transformPortfolioToAssets(), compute totals
App->>User: render AssetsList & WalletAddress
User->>App: Click asset
App->>Modal: open with asset
User->>Modal: enter recipient & amount
Modal->>Blockchain: getCurrentChainId(provider)
Blockchain->>Provider: eth_chainId
Provider-->>Blockchain: chainId
alt chain mismatch
Modal->>Blockchain: switchChain(asset.chainId, provider)
Blockchain->>Provider: wallet_switchEthereumChain / wallet_addEthereumChain
Provider-->>Blockchain: switched / added
end
User->>Modal: Click Send
Modal->>Blockchain: sendTransaction(asset, recipient, amount, provider)
Blockchain->>Provider: eth_sendTransaction (native or encoded ERC-20)
Provider-->>Blockchain: txHash
Modal->>App: onSuccess(txHash, chainId)
App->>App: add tx (pending), refetch portfolio, poll receipt
App->>User: update TransactionStatus entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-04T14:34:00.373ZApplied to files:
📚 Learning: 2025-08-20T09:14:16.888ZApplied to files:
🧬 Code graph analysis (2)src/apps/key-wallet/components/SendAssetModal.tsx (4)
src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx (1)
🪛 Biome (2.1.2)src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx[error] 156-156: This number literal will lose precision at runtime. The value at runtime will be 0.12345678901234568 (lint/correctness/noPrecisionLoss) ⏰ 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)
🔇 Additional comments (10)
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. Comment |
Deploying x with
|
| Latest commit: |
8bfff01
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fa3d8644.x-e62.pages.dev |
| Branch Preview URL: | https://feat-eoa-app.x-e62.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/apps/key-wallet/utils/blockchain.ts (1)
92-107: Apply the same type safety improvement as suggested for switchChain.The walletProvider should be properly typed. See the earlier comment on switchChain for a suggested EIP1193Provider type.
🧹 Nitpick comments (3)
src/apps/key-wallet/utils/blockchain.ts (3)
39-87: Type the walletProvider parameter for better type safety.The function logic and error handling are solid, but
walletProvider: anyloses type safety. Consider using a proper EIP-1193 provider type.Create a type definition or import from viem:
interface EIP1193Provider { request: (args: { method: string; params?: any[] }) => Promise<any>; } export const switchChain = async ( chainId: number, walletProvider: EIP1193Provider ): Promise<void> => { // ... }
159-178: Consider returning null instead of empty string for unsupported chains.Returning an empty string for unsupported chains or when Gnosis is disabled makes it harder for consumers to distinguish between "no explorer available" and an actual error.
-export const getBlockExplorerUrl = (chainId: number, txHash: string): string => { +export const getBlockExplorerUrl = (chainId: number, txHash: string): string | null => { switch (chainId) { case 1: return `https://etherscan.io/tx/${txHash}`; // ... other cases case 100: - return isGnosisEnabled ? `https://gnosisscan.io/tx/${txHash}` : ''; + return isGnosisEnabled ? `https://gnosisscan.io/tx/${txHash}` : null; // ... other cases default: - return ''; + return null; } };
192-194: Consider adding length validation for edge cases.While Ethereum addresses are always 42 characters, adding a guard would make the function more robust if called with unexpected input.
export const shortenAddress = (address: string, chars: number = 4): string => { + if (address.length < chars * 2 + 2) { + return address; + } return `${address.slice(0, chars + 2)}...${address.slice(-chars)}`; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/apps/key-wallet/icon.pngis excluded by!**/*.pngsrc/apps/key-wallet/images/logo-unknown.pngis excluded by!**/*.png
📒 Files selected for processing (14)
src/apps/key-wallet/README.md(1 hunks)src/apps/key-wallet/components/AssetRow.tsx(1 hunks)src/apps/key-wallet/components/AssetsList.tsx(1 hunks)src/apps/key-wallet/components/SearchAssets.tsx(1 hunks)src/apps/key-wallet/components/SendAssetModal.tsx(1 hunks)src/apps/key-wallet/components/TransactionStatus.tsx(1 hunks)src/apps/key-wallet/components/WalletAddress.tsx(1 hunks)src/apps/key-wallet/index.tsx(1 hunks)src/apps/key-wallet/manifest.json(1 hunks)src/apps/key-wallet/styles/tailwindKeyWallet.css(1 hunks)src/apps/key-wallet/tailwind.keywallet.config.js(1 hunks)src/apps/key-wallet/types/index.ts(1 hunks)src/apps/key-wallet/utils/blockchain.ts(1 hunks)src/apps/key-wallet/utils/portfolio.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/apps/key-wallet/components/TransactionStatus.tsxsrc/apps/key-wallet/index.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/key-wallet/components/AssetsList.tsxsrc/apps/key-wallet/components/SearchAssets.tsx
🧬 Code graph analysis (8)
src/apps/key-wallet/components/TransactionStatus.tsx (2)
src/apps/key-wallet/types/index.ts (1)
TransactionStatus(21-26)src/apps/key-wallet/utils/blockchain.ts (2)
getBlockExplorerUrl(159-178)shortenAddress(192-194)
src/apps/key-wallet/components/AssetRow.tsx (2)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)src/apps/key-wallet/utils/blockchain.ts (2)
formatBalance(180-184)formatUsdValue(186-190)
src/apps/key-wallet/index.tsx (2)
src/apps/key-wallet/types/index.ts (2)
Asset(1-14)TransactionStatus(21-26)src/apps/key-wallet/utils/portfolio.ts (2)
transformPortfolioToAssets(14-51)getTotalPortfolioValue(53-55)
src/apps/key-wallet/components/AssetsList.tsx (2)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)src/apps/key-wallet/utils/blockchain.ts (1)
formatUsdValue(186-190)
src/apps/key-wallet/components/SearchAssets.tsx (1)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
src/apps/key-wallet/components/SendAssetModal.tsx (2)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)src/apps/key-wallet/utils/blockchain.ts (6)
getCurrentChainId(92-107)getChainById(24-26)switchChain(39-87)sendTransaction(109-157)formatBalance(180-184)formatUsdValue(186-190)
src/apps/key-wallet/utils/blockchain.ts (1)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
src/apps/key-wallet/utils/portfolio.ts (2)
src/types/api.ts (1)
PortfolioData(739-748)src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
🔇 Additional comments (10)
src/apps/key-wallet/README.md (1)
1-105: LGTM!The documentation is comprehensive and well-structured, providing clear guidance on features, components, usage, and technical details.
src/apps/key-wallet/manifest.json (1)
1-10: LGTM!The manifest structure is correct and includes proper localization support.
src/apps/key-wallet/types/index.ts (1)
1-27: LGTM!The type definitions are well-structured and comprehensive for managing assets, send operations, and transaction tracking.
src/apps/key-wallet/components/TransactionStatus.tsx (1)
1-87: LGTM!The component is well-structured with proper status handling, accessibility attributes (rel="noopener noreferrer"), and clean separation of concerns. The early return pattern and conditional rendering are appropriate.
src/apps/key-wallet/components/SearchAssets.tsx (1)
37-37: Review comment is incorrect—no action required.The parent component (
AssetsList.tsx) properly memoizeshandleFilteredAssetsChangeusinguseCallbackwith an empty dependency array (line 27–29), ensuring a stable function reference across renders. IncludingonFilteredAssetsChangein the SearchAssets dependency array is both safe and appropriate. No infinite loop risk exists.Likely an incorrect or invalid review comment.
src/apps/key-wallet/utils/blockchain.ts (5)
1-14: LGTM! Clean imports.The imports are well-organized and appropriate. Using viem for blockchain interactions is a solid choice.
16-22: LGTM! Feature flag implementation is appropriate.The Gnosis feature flag pattern and chain filtering logic work correctly.
28-33: LGTM! Native asset detection is correct.The function properly handles both zero address and the EEE...EEE sentinel value used by some APIs.
180-184: LGTM! Balance formatting handles edge cases well.The function appropriately handles zero and very small balances with clear display values.
186-190: LGTM! USD value formatting is well-implemented.The function handles edge cases and provides consistent currency formatting.
| @@ -0,0 +1,42 @@ | |||
| import { useState } from 'react'; | |||
| import { shortenAddress } from '../utils/blockchain'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
The shortenAddress utility is imported but never used in this component. The address is displayed in full on Line 25.
Apply this diff:
-import { shortenAddress } from '../utils/blockchain';📝 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.
| import { shortenAddress } from '../utils/blockchain'; |
🤖 Prompt for AI Agents
In src/apps/key-wallet/components/WalletAddress.tsx around line 2, the import
"shortenAddress" from '../utils/blockchain' is unused; remove that import
statement to clean up unused code and avoid lint warnings. Ensure no other
references to shortenAddress remain in the file and save the file.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/key-wallet/components/WalletAddress.tsx(1 hunks)src/apps/key-wallet/index.tsx(1 hunks)src/apps/key-wallet/styles/tailwindKeyWallet.css(1 hunks)src/apps/key-wallet/tailwind.keywallet.config.js(1 hunks)src/apps/key-wallet/utils/blockchain.ts(1 hunks)src/utils/media.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/apps/key-wallet/components/WalletAddress.tsx
- src/apps/key-wallet/styles/tailwindKeyWallet.css
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/apps/key-wallet/index.tsx
🧬 Code graph analysis (2)
src/apps/key-wallet/index.tsx (3)
src/apps/key-wallet/types/index.ts (2)
Asset(1-14)TransactionStatus(21-26)src/apps/key-wallet/utils/portfolio.ts (2)
transformPortfolioToAssets(14-51)getTotalPortfolioValue(53-55)src/apps/key-wallet/utils/blockchain.ts (1)
getChainById(24-32)
src/apps/key-wallet/utils/blockchain.ts (2)
src/utils/blockchain.ts (1)
isGnosisEnabled(41-42)src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
| const chain = getChainById(chainId); | ||
| const publicClient = createPublicClient({ chain, transport: http() }); | ||
|
|
There was a problem hiding this comment.
Point the public client at a real RPC.
http() with no URL dials http://127.0.0.1:8545, so waitForTransactionReceipt always fails outside local dev and you end up marking successful transactions as failed. Please resolve the chain’s RPC URL before instantiating the client.
- const chain = getChainById(chainId);
- const publicClient = createPublicClient({ chain, transport: http() });
+ const chain = getChainById(chainId);
+ const rpcUrl =
+ chain.rpcUrls.public?.http?.[0] ?? chain.rpcUrls.default.http?.[0];
+ if (!rpcUrl) {
+ console.error(`No RPC URL configured for ${chain.name}`);
+ return;
+ }
+ const publicClient = createPublicClient({ chain, transport: http(rpcUrl) });🤖 Prompt for AI Agents
In src/apps/key-wallet/index.tsx around lines 123 to 125, the public client is
created with http() which defaults to localhost; resolve the chain's RPC URL
first (e.g., from chain.rpcUrls.default.http[0] or a configured RPC mapping) and
pass that URL into the transport (http({ url })) when calling
createPublicClient; include a sensible fallback or throw/log an error if no RPC
URL is available so waitForTransactionReceipt points at a real node.
| const balanceInWei = parseUnits(asset.balance.toString(), asset.decimals); | ||
|
|
||
| // Ensure amount doesn't exceed balance | ||
| if (amountInWei > balanceInWei) { | ||
| throw new Error('Insufficient balance'); | ||
| } |
There was a problem hiding this comment.
Don’t feed scientific-notation balances into parseUnits.
asset.balance is a JS number; small values become strings like 1e-7, which parseUnits rejects. That throws for many ERC‑20s with tiny balances and blocks transfers. Please carry the on-chain balance in an exact string/BigInt (e.g., store the raw wei amount on Asset and reuse it here) or otherwise normalize the decimal string before calling parseUnits, so the comparison always works for subunit balances.
🤖 Prompt for AI Agents
In src/apps/key-wallet/utils/blockchain.ts around lines 140 to 145,
asset.balance is a JS number that can be serialized to scientific notation (e.g.
"1e-7") which parseUnits rejects; update the code to use an exact on‑chain
representation (e.g. add/consume an Asset field like balanceWei or balanceRaw as
a string or BigInt containing the raw wei amount) or otherwise convert the JS
number to a precise non‑scientific decimal string before calling parseUnits.
Concretely: stop passing a JS number into parseUnits, read or add the raw wei
string/BigInt on Asset and compare using ethers BigNumber (or normalize with a
safe decimal library) so the amountInWei vs balanceInWei comparison uses exact
integers and never receives scientific-notation strings.
| // Create public client for gas estimation | ||
| const publicClient = createPublicClient({ | ||
| chain, | ||
| transport: http(), | ||
| }); |
There was a problem hiding this comment.
Use an actual RPC endpoint for gas estimation.
http() without arguments still points to localhost, so both gas estimation and subsequent sends explode once you leave a local node. Resolve the chain’s configured RPC (public/default) and pass it to http(...) before creating the public client.
- const publicClient = createPublicClient({
- chain,
- transport: http(),
- });
+ const rpcUrl =
+ chain.rpcUrls.public?.http?.[0] ?? chain.rpcUrls.default.http?.[0];
+ if (!rpcUrl) {
+ throw new Error(`No RPC URL configured for ${chain.name}`);
+ }
+ const publicClient = createPublicClient({
+ chain,
+ transport: http(rpcUrl),
+ });📝 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.
| // Create public client for gas estimation | |
| const publicClient = createPublicClient({ | |
| chain, | |
| transport: http(), | |
| }); | |
| // Create public client for gas estimation | |
| const rpcUrl = | |
| chain.rpcUrls.public?.http?.[0] ?? chain.rpcUrls.default.http?.[0]; | |
| if (!rpcUrl) { | |
| throw new Error(`No RPC URL configured for ${chain.name}`); | |
| } | |
| const publicClient = createPublicClient({ | |
| chain, | |
| transport: http(rpcUrl), | |
| }); |
🤖 Prompt for AI Agents
In src/apps/key-wallet/utils/blockchain.ts around lines 161 to 165, the public
client is created with http() which defaults to localhost; change it to resolve
the chain's configured RPC endpoint (prefer the chain's public or default RPC
URL) and pass that URL into http(<url>) when creating the public client; ensure
you pick a safe fallback if the chain object has no RPC configured (throw or use
a configured global default) and validate the URL before calling
createPublicClient.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/media.ts (1)
26-26: Safari/iOS < 14 compatibility issue.As previously noted,
addEventListeneris not supported in Safari/iOS < 14. The code needs to detect and fall back to the legacyaddListener/removeListenermethods whenaddEventListeneris unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/media.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (3)
src/utils/media.ts (3)
3-7: LGTM!The SSR guard and fallback logic are appropriate.
32-32: LGTM!Standard mobile breakpoint definition.
34-36: LGTM!Clean convenience wrappers for the mobile breakpoint.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx (2)
38-44: Consider a more robust mocking strategy for mobile tests.The
mockReturnValueOnce(true)approach after a dynamic import may be fragile if the component callsuseIsMobilemultiple times or if test execution order varies. Consider resetting and configuring the mock explicitly within this test:it('displays shortened address on mobile', async () => { const { useIsMobile } = await import('../../../../utils/media'); - vi.mocked(useIsMobile).mockReturnValueOnce(true); + vi.mocked(useIsMobile).mockReturnValue(true); render(<WalletAddress address={mockAddress} />); expect(screen.getByText(mockShortenedAddress)).toBeInTheDocument(); + + // Reset to default for subsequent tests + vi.mocked(useIsMobile).mockReturnValue(false); });Alternatively, wrap the mock setup in a helper function to make desktop/mobile test contexts more explicit.
109-135: Consider verifying UI state on clipboard failure.The error handling test properly verifies that errors are logged, but doesn't check the UI state. Consider adding an assertion to ensure the "Copied" state doesn't appear when the clipboard operation fails:
// Verify the error was logged expect(mockWriteText).toHaveBeenCalledWith(mockAddress); expect(consoleErrorSpy).toHaveBeenCalledWith( 'Failed to copy address:', expect.any(Error) ); + + // Verify "Copied" state is not shown on error + expect(screen.queryByText(/copied/i)).not.toBeInTheDocument(); consoleErrorSpy.mockRestore();This ensures users don't receive misleading feedback when the copy operation fails.
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (1)
21-34: Consider whether snapshot testing adds value here.Snapshot tests can be brittle and may pass even when bugs are introduced if snapshots are updated without careful review. Consider replacing with more specific assertions that validate the actual behavior and structure you care about.
src/apps/key-wallet/utils/__tests__/blockchain.test.ts (1)
1-1: Remove unusedafterEachimport.The
afterEachhook is imported but never used in the test suite.Apply this diff:
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest';src/apps/key-wallet/utils/__tests__/portfolio.test.ts (2)
186-214: Consider using proper typing instead ofas any.While using
null as anyat lines 196-197 effectively tests the edge case of missing price values, it bypasses TypeScript's type safety. Consider using a more type-safe approach, such as creating a partial object or using optional properties if thePortfolioDatatype supports them.Alternative approach:
- price: null as any, - price_change_24h: null as any, + price: undefined, + price_change_24h: undefined,Or explicitly define the test type:
- } as PortfolioData; + } as Partial<PortfolioData> as PortfolioData;
157-184: Add defensive validation for chainId parsing; test malformed formats.The concern is valid but incomplete. The implementation at portfolio.ts line 26 uses
contract.chainId.split(':')[1]without guards, and identical unguarded parsing exists in pillarXApiWalletPortfolio.ts line 31-38. Currently only valid "eip155:XXX" format is tested.While the code is defensively safe (malformed input converts to NaN → 'Unknown' chain name), explicit validation and test coverage for edge cases would improve clarity and maintainability:
- Missing ':' separator →
split(':')[1]returnsundefined→Number(undefined)→NaN- Non-numeric suffix →
Number('abc')→NaN- Both cases safely fall back to 'Unknown' but are untested
Add tests for malformed formats (missing ':', non-numeric values, empty strings) and consider adding explicit format validation before parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
src/apps/key-wallet/components/__tests__/__snapshots__/AssetRow.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/AssetsList.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/SearchAssets.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/SendAssetModal.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/TransactionStatus.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/WalletAddress.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokenMarketDataRow/tests/__snapshots__/LeftColumnTokenMarketDataRow.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensWithMarketDataTile/test/__snapshots__/TokensWithMarketDataTile.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
src/apps/key-wallet/components/SearchAssets.tsx(1 hunks)src/apps/key-wallet/components/__tests__/AssetRow.test.tsx(1 hunks)src/apps/key-wallet/components/__tests__/AssetsList.test.tsx(1 hunks)src/apps/key-wallet/components/__tests__/SearchAssets.test.tsx(1 hunks)src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx(1 hunks)src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx(1 hunks)src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx(1 hunks)src/apps/key-wallet/utils/__tests__/blockchain.test.ts(1 hunks)src/apps/key-wallet/utils/__tests__/portfolio.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/key-wallet/components/SearchAssets.tsx
🧬 Code graph analysis (5)
src/apps/key-wallet/utils/__tests__/blockchain.test.ts (1)
src/apps/key-wallet/utils/blockchain.ts (9)
getChainById(24-32)isNativeAsset(34-39)switchChain(45-93)getCurrentChainId(98-113)sendTransaction(115-223)getBlockExplorerUrl(225-244)formatBalance(246-250)formatUsdValue(252-256)shortenAddress(258-260)
src/apps/key-wallet/utils/__tests__/portfolio.test.ts (2)
src/apps/key-wallet/utils/portfolio.ts (3)
transformPortfolioToAssets(14-51)getTotalPortfolioValue(53-55)groupAssetsByChain(57-71)src/types/api.ts (1)
PortfolioData(739-748)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (1)
src/apps/key-wallet/types/index.ts (1)
TransactionStatus(21-26)
src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx (1)
src/utils/media.ts (1)
useIsMobile(34-34)
src/apps/key-wallet/components/SearchAssets.tsx (1)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (17)
src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx (2)
1-26: LGTM: Clean test setup with proper mocking and lifecycle hooks.The test setup properly mocks the clipboard API and media query utilities, with good cleanup in afterEach.
46-100: Excellent coverage of copy functionality with proper async handling.The tests properly handle asynchronous clipboard operations, use fake timers to test timeout behavior, and correctly wrap state updates in
act(). The restoration of real timers ensures no side effects on subsequent tests.src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (4)
1-12: LGTM!The test setup follows standard patterns with proper mock initialization and cleanup.
36-82: LGTM!The status variant tests comprehensively cover pending, success, and failed states with appropriate assertions.
116-152: LGTM!Block explorer link tests properly validate chain-specific URLs for both Ethereum and Polygon networks.
173-194: LGTM!The multiple transactions test effectively verifies that the component handles rendering multiple transaction statuses with independent clear buttons.
src/apps/key-wallet/components/__tests__/AssetRow.test.tsx (1)
29-142: Excellent test coverage!The test suite comprehensively covers rendering, snapshots, asset display, balance formatting, price change styling, logo handling with fallbacks, click interactions, and graceful handling of missing data.
src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx (1)
1-421: Comprehensive test suite with excellent coverage!The test suite thoroughly covers modal behavior including null asset handling, display, validation (address, amount, balance), USD estimation, successful transactions, chain switching, error handling, and loading states. The mocking strategy is appropriate and the async test patterns are correctly implemented.
src/apps/key-wallet/utils/__tests__/blockchain.test.ts (1)
32-361: Excellent test coverage for blockchain utilities!The test suite comprehensively covers all exported utilities including chain management, native asset detection, chain switching with various error scenarios, transaction sending for both native and ERC-20 tokens, block explorer URLs, formatting functions, and address shortening. The mocking strategy appropriately isolates viem dependencies.
src/apps/key-wallet/components/__tests__/AssetsList.test.tsx (1)
1-160: Well-structured test suite with good coverage!The test suite effectively covers loading states, empty states, portfolio value display, asset count variations (singular/plural), asset rendering, and search component integration. The test structure is clear and assertions are appropriate.
src/apps/key-wallet/components/__tests__/SearchAssets.test.tsx (1)
1-217: Thorough test coverage for search functionality!The test suite comprehensively covers search input rendering, filtering by multiple fields (symbol, name, chain), fuzzy search behavior, clear button functionality, query display, and edge cases like empty assets. The tests properly verify the callback mechanism and filtering results.
src/apps/key-wallet/components/SearchAssets.tsx (2)
43-97: Clean and accessible search UI!The component provides a well-structured search interface with proper accessibility attributes (
aria-labelon the clear button), visual feedback (search icon, clear button), and user-friendly features (showing current search query). The styling uses consistent design tokens.
28-37: Callback memoization verified—no issues found.Verification confirms that the parent component (
AssetsList) properly memoizesonFilteredAssetsChangeusinguseCallbackwith an empty dependency array. The callback reference is stable across renders, so includingonFilteredAssetsChangein theSearchAssetsuseEffect dependency array will not cause unnecessary re-runs or performance issues.src/apps/key-wallet/utils/__tests__/portfolio.test.ts (4)
1-9: LGTM!The test structure and imports are well-organized. The file properly imports the utilities under test and follows vitest conventions.
10-155: Excellent test coverage for the main scenarios.The test suite covers the critical paths including undefined inputs, empty data, zero-balance filtering, sorting, and the transformation logic itself. The assertions are precise and the test data is realistic.
217-257: LGTM!The test suite for
getTotalPortfolioValueappropriately covers both the empty array edge case and the calculation logic with multiple assets. The assertions are clear and correct.
259-317: LGTM!The test suite for
groupAssetsByChaincovers the essential scenarios including empty input and multi-chain grouping. The test correctly verifies that assets are properly grouped by chain name while preserving references to the original asset objects.
| @@ -0,0 +1,142 @@ | |||
| import { describe, it, expect, vi } from 'vitest'; | |||
There was a problem hiding this comment.
Missing beforeEach import.
Line 25 uses beforeEach but it's not included in the imports from vitest. This will cause a runtime error.
Apply this diff:
-import { describe, it, expect, vi } from 'vitest';
+import { describe, it, expect, vi, beforeEach } from 'vitest';📝 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.
| import { describe, it, expect, vi } from 'vitest'; | |
| import { describe, it, expect, vi, beforeEach } from 'vitest'; |
🤖 Prompt for AI Agents
In src/apps/key-wallet/components/__tests__/AssetRow.test.tsx around lines 1 to
1, the test uses beforeEach on line 25 but the import from 'vitest' at the top
does not include beforeEach; update the import statement to include beforeEach
(e.g., add beforeEach to the destructured imports alongside describe, it,
expect, vi) so the runtime error is resolved.
| it('displays shortened transaction hash', () => { | ||
| const transactions: TxStatus[] = [ | ||
| { | ||
| hash: '0x1234567890123456789012345678901234567890123456789012345678901234', | ||
| chainId: 1, | ||
| status: 'pending', | ||
| timestamp: Date.now(), | ||
| }, | ||
| ]; | ||
|
|
||
| render(<TransactionStatus transactions={transactions} onClear={mockOnClear} />); | ||
|
|
||
| // Should show shortened hash (0x12345678...56789012) | ||
| expect(screen.getByText(/0x12345678/)).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Strengthen the shortened hash assertion.
The test only verifies that "0x12345678" appears but doesn't confirm the full shortened format mentioned in the comment (0x12345678...56789012). Consider testing for the complete pattern including the ellipsis and ending portion.
Apply this diff to improve the assertion:
- // Should show shortened hash (0x12345678...56789012)
- expect(screen.getByText(/0x12345678/)).toBeInTheDocument();
+ // Should show shortened hash (0x12345678...56789012)
+ expect(screen.getByText(/0x12345678.*56789012/)).toBeInTheDocument();📝 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.
| it('displays shortened transaction hash', () => { | |
| const transactions: TxStatus[] = [ | |
| { | |
| hash: '0x1234567890123456789012345678901234567890123456789012345678901234', | |
| chainId: 1, | |
| status: 'pending', | |
| timestamp: Date.now(), | |
| }, | |
| ]; | |
| render(<TransactionStatus transactions={transactions} onClear={mockOnClear} />); | |
| // Should show shortened hash (0x12345678...56789012) | |
| expect(screen.getByText(/0x12345678/)).toBeInTheDocument(); | |
| }); | |
| it('displays shortened transaction hash', () => { | |
| const transactions: TxStatus[] = [ | |
| { | |
| hash: '0x1234567890123456789012345678901234567890123456789012345678901234', | |
| chainId: 1, | |
| status: 'pending', | |
| timestamp: Date.now(), | |
| }, | |
| ]; | |
| render(<TransactionStatus transactions={transactions} onClear={mockOnClear} />); | |
| // Should show shortened hash (0x12345678...56789012) | |
| expect(screen.getByText(/0x12345678.*56789012/)).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
In src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx around
lines 84 to 98, the test currently only asserts that "0x12345678" appears;
update the assertion to verify the full shortened hash format (e.g.
"0x12345678...56789012") by matching the complete pattern including the ellipsis
and trailing characters — use a regex that escapes the dots (for example
/0x12345678\.\.\.56789012/) or otherwise assert the element's textContent equals
the expected shortened string so the test confirms both start, ellipsis, and end
portions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (2)
98-112: Shortened hash assertion still incomplete.The test continues to verify only the hash prefix (
/0x12345678/) rather than the complete shortened format including the ellipsis and ending portion (e.g.,0x12345678...56789012) mentioned in the comment on line 110. This limitation could mask bugs in the hash formatting logic.
167-184: Hash format inconsistency remains unaddressed.The test still uses a short hash
'0xtxhash'on line 170, which is inconsistent with the full 64-character hashes (66 characters including the0xprefix) used in other tests throughout this file. This inconsistency could mask bugs if theTransactionStatuscomponent performs validation or formatting based on hash length.
🧹 Nitpick comments (1)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (1)
186-207: Consider using full-length hashes for consistency.Similar to the concern raised for the
onCleartest, this test uses short hashes ('0xhash1'and'0xhash2') instead of the full 64-character format used in other tests. While the short hashes make it easier to distinguish between transactions in this specific test scenario, using consistent full-length hashes across all tests would better verify that the component handles standard transaction hash formats correctly and doesn't have hidden length-dependent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/key-wallet/components/__tests__/__snapshots__/TransactionStatus.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
🧬 Code graph analysis (1)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (1)
src/apps/key-wallet/types/index.ts (1)
TransactionStatus(21-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (1)
src/apps/key-wallet/components/__tests__/TransactionStatus.test.tsx (1)
16-26: Excellent fix for timestamp test determinism!The addition of Date mocking in the test setup directly addresses the previous concern about environment-dependent timestamp tests. By mocking both
Date.now()andDate.prototype.toLocaleString()to return fixed values, the tests are now deterministic and will produce consistent results across different CI environments and developer machines with varying locale/timezone settings.
src/apps/key-wallet/README.md
Outdated
| - Mobile-first responsive design | ||
| - Clean, modern UI using Tailwind CSS | ||
| - Master-detail view pattern (list → detail modal) | ||
| - Accessible and intuitive navigation |
There was a problem hiding this comment.
Is it really "accessible"?
There was a problem hiding this comment.
I'm not sure but i have removed it - not sure this whole README is needed
There was a problem hiding this comment.
Ok thanks, yes we do not have README for other apps within PillarX
| // types | ||
| import { Asset } from '../types'; | ||
|
|
||
| const isGnosisEnabled = import.meta.env.VITE_FEATURE_FLAG_GNOSIS === 'true'; |
There was a problem hiding this comment.
This utils feels very familiar, I think most of those function exists elsewhere in the repo, this is repeated code
There was a problem hiding this comment.
Ah yes i have updated this 👍
| @@ -0,0 +1,36 @@ | |||
| import { useEffect, useState } from 'react'; | |||
|
|
|||
| export const isClientMatchMedia = (query: string): boolean => { | |||
There was a problem hiding this comment.
Do we actually need media queries here? Usually media queries can be managed by the screen sizes set in tailwind (desktop, mobile etc.)
There was a problem hiding this comment.
Yes good point i've changed to Tailwind breakpoints but left this media utility in for JS based detection should it be needed
There was a problem hiding this comment.
ok hopefully both will not clash
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/apps/key-wallet/components/SendAssetModal.tsx (5)
80-84: Optional: Remove redundant null coalescing.The nullish coalescing on line 83 (
delegatedChainId ?? null) is redundant becausedelegatedChainIdis already guaranteed to be either a number ornullfrom line 82.Apply this diff to simplify:
if (isDelegatedEoa && kit && asset) { const delegatedChainId = kit.getEtherspotProvider?.().getChainId?.() ?? asset.chainId ?? null; - setCurrentChainId(delegatedChainId ?? null); + setCurrentChainId(delegatedChainId); return; }
119-122: Prefer viem'sisAddressfor address validation.The regex validation is less robust than viem's
isAddressfunction, which is already imported inblockchain.tsand handles checksum validation. For consistency and correctness, consider usingisAddresshere.You can import it at the top:
// Viem -import { encodeFunctionData, erc20Abi, parseUnits } from 'viem'; +import { encodeFunctionData, erc20Abi, parseUnits, isAddress } from 'viem';Then use it in validation:
- if (!recipient.match(/^0x[a-fA-F0-9]{40}$/)) { + if (!isAddress(recipient)) { setError('Invalid Ethereum address'); return false; }
139-139: Consider a more concise error message.The error message is quite verbose for a user-facing error. Users typically don't need to know internal details like "PillarX is not connected." A simpler message like "Wallet not connected. Please reload or try logging in again." would be clearer.
- throw new Error('Sorry, PillarX is not connected to a wallet - please try reloading the site or logging out and back in.'); + throw new Error('Wallet not connected. Please reload the page or try logging in again.');
189-195: Extract magic numbers to named constants.The timeout values
120000(120 seconds) and3000(3 seconds) would be more maintainable as named constants.At the top of the file or in a constants section:
const TX_HASH_TIMEOUT_MS = 120000; // 2 minutes const TX_HASH_POLL_INTERVAL_MS = 3000; // 3 secondsThen use them:
const txHash = (await kit.getTransactionHash( userOpHash, asset.chainId, - 120000, - 3000 + TX_HASH_TIMEOUT_MS, + TX_HASH_POLL_INTERVAL_MS )) || undefined;
354-362: Consider usingtype="text"for better UX.Using
type="number"for amount inputs can cause UX issues: scientific notation display (e.g.,1e-7), accidental scroll-wheel changes, and inconsistent browser behavior. Many crypto apps usetype="text"withinputMode="decimal"and pattern validation for better control.<input - type="number" + type="text" + inputMode="decimal" value={amount} onChange={(e) => setAmount(e.target.value)} placeholder="0.0" - step="any" className="w-full px-4 py-3 bg-white/5 border border-white/10 rounded-xl text-white placeholder-white/40 focus:outline-none focus:border-purple_medium transition-colors pr-20" disabled={isLoading} />You may also want to add input sanitization in the
onChangehandler to allow only valid decimal numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/apps/key-wallet/components/__tests__/__snapshots__/SendAssetModal.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/TransactionStatus.test.tsx.snapis excluded by!**/*.snapsrc/apps/key-wallet/components/__tests__/__snapshots__/WalletAddress.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/apps/key-wallet/components/SendAssetModal.tsx(1 hunks)src/apps/key-wallet/components/TransactionStatus.tsx(1 hunks)src/apps/key-wallet/components/WalletAddress.tsx(1 hunks)src/apps/key-wallet/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/apps/key-wallet/index.tsx
- src/apps/key-wallet/components/WalletAddress.tsx
- src/apps/key-wallet/components/TransactionStatus.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/key-wallet/components/SendAssetModal.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/apps/key-wallet/components/SendAssetModal.tsx
🧬 Code graph analysis (1)
src/apps/key-wallet/components/SendAssetModal.tsx (3)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)src/apps/key-wallet/utils/blockchain.ts (7)
getCurrentChainId(126-145)getChainById(42-50)isNativeAsset(52-57)switchChain(63-121)sendTransaction(147-277)formatBalance(300-304)formatUsdValue(306-310)src/utils/eip7702Authorization.ts (1)
getEIP7702AuthorizationIfNeeded(16-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/apps/key-wallet/utils/blockchain.ts (2)
172-178: Fix scientific notation issue in balance comparison.
asset.balanceis a JavaScript number that serializes to scientific notation for small values (e.g.,0.0000001becomes"1e-7"), whichparseUnitsrejects. This will cause transaction failures for tokens with tiny balances.Solution: Store and use the raw on-chain balance (in wei/smallest unit) as a string or BigInt on the
Assettype, then compareamountInWeidirectly against that exact value.Update the
Assetinterface insrc/apps/key-wallet/types/index.ts:export interface Asset { id: number; name: string; symbol: string; logo: string; balance: number; + balanceRaw: string; // Raw balance in wei/smallest unit as string decimals: number; price: number; price_change_24h: number; contract: string; chainId: number; chainName: string; usdBalance: number; }Then update this code:
- // Convert asset balance to wei for comparison - const balanceInWei = parseUnits(asset.balance.toString(), asset.decimals); + // Use raw balance (already in wei) for comparison + const balanceInWei = BigInt(asset.balanceRaw); // Ensure amount doesn't exceed balance if (amountInWei > balanceInWei) { throw new Error('Insufficient balance'); }
214-218: Use the chain's configured RPC endpoint for gas estimation.
http()without arguments defaults tohttp://127.0.0.1:8545, causing gas estimation and transactions to fail outside local development environments.Apply this diff:
// Create public client for gas estimation + const rpcUrl = + chain.rpcUrls.public?.http?.[0] ?? chain.rpcUrls.default.http[0]; + if (!rpcUrl) { + throw new Error(`No RPC URL configured for chain ${chain.name}`); + } const publicClient = createPublicClient({ chain, - transport: http(), + transport: http(rpcUrl), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/key-wallet/utils/blockchain.ts(1 hunks)src/utils/blockchain.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/key-wallet/utils/blockchain.ts
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 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/apps/key-wallet/utils/blockchain.ts
🧬 Code graph analysis (1)
src/apps/key-wallet/utils/blockchain.ts (2)
src/utils/blockchain.ts (2)
isGnosisEnabled(41-42)getBlockScan(225-246)src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (2)
src/apps/key-wallet/utils/blockchain.ts (1)
281-284: Good refactoring to reuse shared utility.The function now properly delegates to
getBlockScanfrom the shared utilities, eliminating the duplicated chain-to-explorer mapping. This addresses the duplication concern raised in previous reviews.src/utils/blockchain.ts (1)
242-242: LGTM!The URL protocol update to HTTPS ensures secure connections to Arbiscan and aligns with security best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx (1)
97-123: Robust error handling test with proper spy management.The test correctly mocks a clipboard failure, verifies error logging, and properly restores the console.error spy. The double
Promise.resolve()on lines 112-113 ensures the catch block has executed.Consider replacing the double
Promise.resolve()pattern withwaitForfor better clarity:- // Wait for the async promise rejection to be handled - await Promise.resolve(); - await Promise.resolve(); // Need to flush the catch block execution - - // Verify the error was logged - expect(mockWriteText).toHaveBeenCalledWith(mockAddress); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Failed to copy address:', - expect.any(Error) - ); + // Wait for the async promise rejection to be handled + await waitFor(() => { + expect(mockWriteText).toHaveBeenCalledWith(mockAddress); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Failed to copy address:', + expect.any(Error) + ); + });This makes the intent clearer and is more resilient to timing changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/key-wallet/components/__tests__/__snapshots__/WalletAddress.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/apps/key-wallet/components/WalletAddress.tsx(1 hunks)src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/apps/key-wallet/components/WalletAddress.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (4)
src/apps/key-wallet/components/__tests__/WalletAddress.test.tsx (4)
1-21: LGTM! Clean test setup with proper mock lifecycle.The beforeEach/afterEach hooks correctly initialize and clean up the clipboard mock, ensuring test isolation.
23-37: Good coverage of basic rendering scenarios.The tests verify essential UI elements including both address formats (responsive design) and use accessible role queries for the button. The snapshot test provides visual regression protection.
39-88: Excellent async and timer handling!The clipboard tests properly handle asynchronous operations with
waitForandact. The timer test (lines 61-88) correctly uses fake timers to verify the 2-second reset behavior, with proper cleanup and microtask flushing.
90-95: LGTM! Clear verification of instructional text.The test ensures the user-facing guidance is present in the component.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/apps/key-wallet/components/SendAssetModal.tsx (1)
221-245: Avoid using error state for chain-switching status.The "Switching to {chain}..." message on lines 229-230 is displayed in the error UI (red box), which confuses users since chain switching is an expected operation, not an error. This creates poor UX.
Consider using a separate info/loading state:
const [error, setError] = useState(''); const [isLoading, setIsLoading] = useState(false); +const [infoMessage, setInfoMessage] = useState(''); // In handleSend: if (currentChainId !== asset.chainId) { - setError( - `Switching to ${targetChain.name}... Please approve in your wallet.` - ); + setInfoMessage( + `Switching to ${targetChain.name}... Please approve in your wallet.` + ); try { await switchChain(asset.chainId, resolvedProvider); - setError(''); + setInfoMessage('');Then render an info box (not error box) for
infoMessagein your JSX.src/apps/key-wallet/utils/blockchain.ts (3)
30-30: Import isGnosisEnabled from shared utilities.The
isGnosisEnabledconstant is duplicated fromsrc/utils/blockchain.ts. Import it alongsidegetBlockScanto maintain a single source of truth.Apply this diff:
-import { getBlockScan } from '../../../utils/blockchain'; +import { getBlockScan, isGnosisEnabled } from '../../../utils/blockchain'; import type { WalletProviderLike, Eip1193LikeProvider, } from '../../../types/walletProvider'; -const isGnosisEnabled = import.meta.env.VITE_FEATURE_FLAG_GNOSIS === 'true'; - const allChains = [mainnet, polygon, gnosis, base, bsc, optimism, arbitrum];Based on learnings
180-186: Critical: Scientific notation breaks balance comparison.Converting
asset.balance(a JS number) to string via.toString()produces scientific notation for small values (e.g.,0.0000001becomes"1e-7"), whichparseUnitsrejects. This prevents transfers of tokens with tiny balances.The
Assettype should store the raw on-chain balance as a string or BigInt to avoid precision loss. For example:// In src/apps/key-wallet/types/index.ts export interface Asset { // ... existing fields balance: number; // Display value balanceRaw?: string; // Raw wei/atomic units as string }Then in
sendTransaction:- const balanceInWei = parseUnits(asset.balance.toString(), asset.decimals); + const balanceInWei = asset.balanceRaw + ? BigInt(asset.balanceRaw) + : parseUnits(asset.balance.toString(), asset.decimals);Alternatively, if
balanceRawcannot be added, normalize the decimal string before callingparseUnitsto avoid scientific notation.
222-226: Critical: http() without RPC URL fails in non-local environments.Calling
http()without arguments defaults to localhost, causing gas estimation to fail in production. Pass the chain's configured RPC endpoint explicitly.Apply this diff:
+ const rpcUrl = + chain.rpcUrls.public?.http?.[0] ?? chain.rpcUrls.default.http?.[0]; + if (!rpcUrl) { + throw new Error(`No RPC URL configured for ${chain.name}`); + } const publicClient = createPublicClient({ chain, - transport: http(), + transport: http(rpcUrl), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/key-wallet/components/__tests__/__snapshots__/WalletAddress.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/apps/key-wallet/components/SendAssetModal.tsx(1 hunks)src/apps/key-wallet/components/WalletAddress.tsx(1 hunks)src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx(1 hunks)src/apps/key-wallet/index.tsx(1 hunks)src/apps/key-wallet/utils/blockchain.ts(1 hunks)src/providers/EtherspotTransactionKitProvider.tsx(3 hunks)src/types/walletProvider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/apps/key-wallet/index.tsx
- src/apps/key-wallet/components/tests/SendAssetModal.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/providers/EtherspotTransactionKitProvider.tsxsrc/apps/key-wallet/components/SendAssetModal.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/providers/EtherspotTransactionKitProvider.tsxsrc/apps/key-wallet/components/SendAssetModal.tsxsrc/apps/key-wallet/utils/blockchain.ts
📚 Learning: 2025-08-12T07:42:24.656Z
Learnt from: IAmKio
Repo: pillarwallet/x PR: 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/apps/key-wallet/utils/blockchain.ts
🧬 Code graph analysis (4)
src/providers/EtherspotTransactionKitProvider.tsx (2)
__mocks__/EtherspotTransactionKitProvider.tsx (1)
EtherspotTransactionKitContextType(5-12)src/types/walletProvider.ts (1)
WalletProviderLike(15-15)
src/apps/key-wallet/components/SendAssetModal.tsx (4)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)src/types/walletProvider.ts (1)
WalletProviderLike(15-15)src/apps/key-wallet/utils/blockchain.ts (7)
getCurrentChainId(133-154)getChainById(47-55)isNativeAsset(57-62)switchChain(68-128)sendTransaction(156-286)formatBalance(294-298)formatUsdValue(300-304)src/utils/eip7702Authorization.ts (1)
getEIP7702AuthorizationIfNeeded(16-95)
src/apps/key-wallet/components/WalletAddress.tsx (1)
src/apps/key-wallet/utils/blockchain.ts (1)
shortenAddress(306-308)
src/apps/key-wallet/utils/blockchain.ts (3)
src/utils/blockchain.ts (2)
isGnosisEnabled(41-42)getBlockScan(225-246)src/types/walletProvider.ts (2)
WalletProviderLike(15-15)Eip1193LikeProvider(8-13)src/apps/key-wallet/types/index.ts (1)
Asset(1-14)
⏰ 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 (13)
src/types/walletProvider.ts (1)
1-15: LGTM! Clean type definitions.The wallet provider types are well-structured and provide good flexibility for supporting both Viem WalletClient and EIP-1193-style providers. The type definitions follow standard patterns and enable safe interoperability.
src/providers/EtherspotTransactionKitProvider.tsx (3)
13-13: LGTM! Context type correctly extended.The import and context type extension properly expose the wallet provider to consuming components.
Also applies to: 20-20
37-41: Type assertion is acceptable for config initialization.The type assertion to access
config.provideris a reasonable approach given that the config type may not explicitly include the provider property. The initialization logic correctly handles the optional provider.
66-72: LGTM! Provider sync effect is correct.The effect correctly synchronizes the external provider state when the config changes, ensuring the exposed
walletProviderremains current.src/apps/key-wallet/components/WalletAddress.tsx (1)
1-61: LGTM! Proper timeout cleanup implemented.The component correctly manages the copy timeout using a ref and cleans it up on unmount. The implementation handles the user experience well.
Note: The
useMemoon line 36 is a minor optimization for a simple string operation and could be removed, but it's not causing any issues.src/apps/key-wallet/components/SendAssetModal.tsx (3)
44-55: LGTM! Provider resolution logic is sound.The component correctly resolves the wallet provider from multiple sources with appropriate fallbacks. The type assertion is necessary given the context typing.
57-66: LGTM! Defensive wallet mode detection.The delegated EOA check uses appropriate optional chaining and error handling to safely determine wallet mode without crashing if methods are unavailable.
138-205: LGTM! Comprehensive delegated EOA transaction flow.The function properly handles the complete transaction lifecycle for delegated EOA wallets, including authorization, estimation, sending, and hash retrieval with appropriate error handling at each step.
src/apps/key-wallet/utils/blockchain.ts (5)
38-62: LGTM! Utility functions are well-implemented.The type guard, chain resolution, and native asset detection are all correctly implemented with appropriate error handling.
68-128: LGTM! Comprehensive chain switching with fallback.The function correctly handles chain switching for EIP-1193 providers and gracefully adds the chain if it's not already in the wallet. Error messages guide users appropriately.
133-154: LGTM! Safe chain ID retrieval.The function correctly handles both provider types and returns
nullon error rather than throwing, which is appropriate for a query operation.
228-286: Transaction flows are well-structured.Both native and ERC-20 send flows properly estimate gas before sending and include comprehensive error handling (aside from the RPC URL issue flagged separately).
289-308: LGTM! Display utilities are well-implemented.The block explorer URL correctly reuses the shared
getBlockScanutility, and the formatting functions provide appropriate user-friendly display thresholds.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/providers/EtherspotTransactionKitProvider.tsx (2)
20-20: Interface update looks good.The addition of
walletProviderto the context data interface correctly exposes the external provider to consumers. The past review comment on lines 76-84 already flagged that the mock file needs updating to match this interface.
76-84: Context data correctly exposes the wallet provider.The memoized context data properly includes
walletProviderand has the correct dependency array. The past review comment on this range already identified that the mock file needs updating to match.src/apps/key-wallet/components/SendAssetModal.tsx (1)
69-408: Stop using the error state for chain-switch info
Repurposing the error UI to display “Switching to …” brings back the confusion flagged in the earlier review—the red error banner fires for a non-error state. Please introduce a neutral info state (e.g.infoMessage) instead of overloadingerror, clear it alongsideerror, and render it with a non-error style. This keeps real errors distinct from expected prompts. See sketch diff below.@@ - const [error, setError] = useState(''); + const [error, setError] = useState(''); + const [infoMessage, setInfoMessage] = useState(''); @@ - setError(''); + setError(''); + setInfoMessage(''); @@ - setError( + setInfoMessage( `Switching to ${targetChain.name}... Please approve in your wallet.` ); @@ - setError(''); // Clear the switching message + setInfoMessage(''); @@ - setError( + setInfoMessage(''); + setError( @@ - onSuccess(txHash, asset.chainId); + onSuccess(txHash, asset.chainId); onClose(); @@ - setError( + setInfoMessage(''); + setError( @@ - {error && ( + {infoMessage && ( + <div className="bg-blue-500/10 border border-blue-500/20 rounded-xl p-3"> + <p className="text-sm text-blue-300">{infoMessage}</p> + </div> + )} + {error && (
🧹 Nitpick comments (1)
src/providers/EtherspotTransactionKitProvider.tsx (1)
37-43: Consider extracting duplicated provider extraction logic.The logic for extracting
providerfromconfigis duplicated between the state initializer (lines 40-42) and the sync effect (lines 69-72). Extract this into a helper function to improve maintainability.Apply this refactor:
+const getProviderFromConfig = (config: EtherspotTransactionKitConfig): WalletProviderLike | undefined => { + return 'provider' in config + ? (config as { provider?: WalletProviderLike }).provider + : undefined; +}; + export const EtherspotTransactionKitProvider: React.FC< EtherspotTransactionKitProviderProps > = ({ config, children }) => { const [walletAddress, setWalletAddress] = useState<string>(); const kitRef = useRef<EtherspotTransactionKit | null>(null); const [externalProvider, setExternalProvider] = useState< WalletProviderLike | undefined - >(() => - 'provider' in config - ? (config as { provider?: WalletProviderLike }).provider - : undefined - ); + >(() => getProviderFromConfig(config)); // ... kit creation ... useEffect(() => { - setExternalProvider( - 'provider' in config - ? (config as { provider?: WalletProviderLike }).provider - : undefined - ); + setExternalProvider(getProviderFromConfig(config)); }, [config]);Also applies to: 68-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apps/key-wallet/components/SendAssetModal.tsx(1 hunks)src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx(1 hunks)src/providers/EtherspotTransactionKitProvider.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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.
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 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/providers/EtherspotTransactionKitProvider.tsxsrc/apps/key-wallet/components/SendAssetModal.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/providers/EtherspotTransactionKitProvider.tsxsrc/apps/key-wallet/components/SendAssetModal.tsx
🧬 Code graph analysis (3)
src/providers/EtherspotTransactionKitProvider.tsx (2)
__mocks__/EtherspotTransactionKitProvider.tsx (1)
EtherspotTransactionKitContextType(5-12)src/types/walletProvider.ts (1)
WalletProviderLike(15-15)
src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx (1)
src/types/walletProvider.ts (1)
WalletProviderLike(15-15)
src/apps/key-wallet/components/SendAssetModal.tsx (4)
src/apps/key-wallet/types/index.ts (1)
Asset(1-14)src/types/walletProvider.ts (1)
WalletProviderLike(15-15)src/apps/key-wallet/utils/blockchain.ts (7)
getCurrentChainId(133-154)getChainById(47-55)isNativeAsset(57-62)switchChain(68-128)sendTransaction(156-286)formatBalance(294-298)formatUsdValue(300-304)src/utils/eip7702Authorization.ts (1)
getEIP7702AuthorizationIfNeeded(16-95)
🪛 Biome (2.1.2)
src/apps/key-wallet/components/__tests__/SendAssetModal.test.tsx
[error] 156-156: This number literal will lose precision at runtime.
The value at runtime will be 0.12345678901234568
(lint/correctness/noPrecisionLoss)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (1)
src/providers/EtherspotTransactionKitProvider.tsx (1)
13-13: LGTM!The import is necessary for exposing the wallet provider in the context.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
UI
Utilities & Types
Documentation
Style
Tests
Chores