Conversation
WalkthroughAdds EIP-5792 batch handling and status tracking to WalletConnect, serializes BigInt for JSON responses, introduces WALLET_* method constants, adds optional batchName to batch data, and removes kit-derived wallet mode checks from WalletConnect dropdown logic in portfolio components. Changes
Sequence Diagram(s)sequenceDiagram
participant DApp as Wallet / DApp
participant WC as WalletConnect Service
participant UI as UI / User
participant Chain as Blockchain
rect rgb(230, 245, 255)
note over WC: EIP-5792-capable request handling
end
DApp->>WC: WALLET_GET_CAPABILITIES(chainIds)
WC->>WC: getWalletCapabilities(chainIds)
WC-->>DApp: capabilities per chain
DApp->>WC: WALLET_SEND_CALLS (single or batch)
WC->>UI: Show confirmation (single vs batch)
UI->>UI: User confirms
alt Single call
WC->>Chain: eth_sendTransaction
Chain-->>WC: txHash
else Batch (multiple calls)
WC->>Chain: prepare & send batch / userOp
Chain-->>WC: txHash and/or userOpHash
end
WC->>WC: store mapping in batchIdToTxDataMap
WC-->>DApp: return batchId
DApp->>WC: WALLET_GET_CALLS_STATUS(batchId)
WC->>WC: getCallsStatus(batchId) -> check userOp / receipt
WC-->>DApp: status (PENDING / CONFIRMED / REJECTED)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: |
f2f9855
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://08592c4a.x-e62.pages.dev |
| Branch Preview URL: | https://pro-3857-fix-wallet-connect.x-e62.pages.dev |
Deploying pillarx-debug with
|
| Latest commit: |
f2f9855
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://993f8d32.pillarx-debug.pages.dev |
| Branch Preview URL: | https://pro-3857-fix-wallet-connect.pillarx-debug.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: 4
🧹 Nitpick comments (3)
src/services/walletConnect.ts (2)
53-57: Potential memory leak: in-memory batch ID map grows unbounded.The
batchIdToTxDataMapMap is never cleaned up. Over time, as users execute transactions, this map will grow indefinitely since entries are only added but never removed.Consider implementing a cleanup strategy:
// In-memory mapping of batch IDs to transaction data -const batchIdToTxDataMap = new Map< +const BATCH_TTL_MS = 30 * 60 * 1000; // 30 minutes +const batchIdToTxDataMap = new Map< string, - { txHash: string; userOpHash?: string } + { txHash: string; userOpHash?: string; createdAt: number } >(); + +// Cleanup old batch entries periodically +const cleanupOldBatches = () => { + const now = Date.now(); + for (const [batchId, data] of batchIdToTxDataMap.entries()) { + if (now - data.createdAt > BATCH_TTL_MS) { + batchIdToTxDataMap.delete(batchId); + } + } +}; +setInterval(cleanupOldBatches, 5 * 60 * 1000); // Run every 5 minutesYou would also need to update
setcalls to includecreatedAt: Date.now().
1590-1604: AsynconSentcallback may fail silently without error handling.The
onSentcallback performs async operations (kit.getTransactionHash) but errors are not caught. IfgetTransactionHashfails, the batch mapping won't be stored, causingwallet_getCallsStatusto return incorrect status.onSent: async (userOpHashes: string[]) => { // Get transaction hash from userOp hash and store it with the batch ID if (userOpHashes && userOpHashes.length > 0) { const userOpHash = userOpHashes[0]; - const txHash = await kit.getTransactionHash( - userOpHash, - callsChainId - ); - if (txHash) { - setWalletConnectTxHash(txHash); - // Store mapping of batchId -> {txHash, userOpHash} for proper wallet_getCallsStatus lookup - batchIdToTxDataMap.set(batchId, { txHash, userOpHash }); + try { + const txHash = await kit.getTransactionHash( + userOpHash, + callsChainId + ); + if (txHash) { + setWalletConnectTxHash(txHash); + batchIdToTxDataMap.set(batchId, { txHash, userOpHash }); + } + } catch (e) { + console.error('Failed to get transaction hash in onSent:', e); + // Still store the userOpHash so status can be checked + batchIdToTxDataMap.set(batchId, { txHash: '', userOpHash }); } } },src/apps/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsx (1)
49-52: Stale comment: mentionsdelegatedEoamode check that was removed.The comment on lines 49-50 states "or if user is in delegatedEoa mode" but the actual wallet mode check has been removed from the condition. Either update the comment to reflect the new logic, or verify the removal was intentional.
- // Don't show WalletConnectDropdown if user is connected via Wagmi or Privy with WalletConnect - // or if user is in delegatedEoa mode + // Don't show WalletConnectDropdown if user is connected via Wagmi or Privy with WalletConnect const shouldShowWalletConnectDropdown = !isConnected && !isPrivyConnectedViaWalletConnect;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsx(2 hunks)src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(3 hunks)src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx(3 hunks)src/services/walletConnect.ts(7 hunks)src/types/index.ts(1 hunks)src/utils/walletConnect.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsxsrc/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsxsrc/services/walletConnect.ts
📚 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/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsxsrc/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
🧬 Code graph analysis (1)
src/services/walletConnect.ts (3)
src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/utils/walletConnect.ts (3)
WALLET_GET_CAPABILITIES(13-13)WALLET_SEND_CALLS(14-14)WALLET_GET_CALLS_STATUS(15-15)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Cloudflare Pages: pillarx-debug
- GitHub Check: build
🔇 Additional comments (12)
src/services/walletConnect.ts (5)
71-86: Returning REJECTED for missing batch may cause incorrect dApp behavior.When
txDatais not found, returningstatus: 'REJECTED'could mislead dApps into thinking the user explicitly rejected the transaction. Per EIP-5792, there's no explicit "not found" status, butPENDINGmight be more appropriate when the batch ID exists but hasn't been processed yet.Consider whether returning
PENDINGinitially would be safer, as the batch ID may simply not have been stored yet due to timing.
1548-1552:txHashretrieval happens after user confirmation timeout but is stored immediately.The code awaits
getTransactionHash()which polls for up to 3 minutes. If the user doesn't confirm in time or rejects,txHashwill beundefined, butbatchIdToTxDataMap.setis only called iftxHashexists. This is correct, but the batch ID has already been returned to the dApp. Subsequentwallet_getCallsStatuscalls will returnREJECTEDuntil the tx is confirmed, which may confuse dApps expecting a pending state.
1626-1645:serializeBigIntshelper is well-implemented.The recursive BigInt serialization handles all cases correctly (null, undefined, arrays, objects, primitives). This ensures JSON serialization won't fail on BigInt values from viem.
1439-1447: The review comment is incorrect. Per EIP-5792,wallet_getCapabilitiesexpects parameters as[Address, string[]], where the chain IDs array is the second parameter (index [1]), not the first.The code at line 1440 correctly accesses
request.params?.[1]for chain IDs. The EIP-5792 specification confirms that the parameter structure is:
params[0]: Wallet address (0x-prefixed hex string)params[1]: Optional array of EIP-155 chain IDs (hex strings)The implementation is correct and follows the specification.
199-236: Based on my verification of the EIP-5792 specification and codebase analysis, I can now provide the rewritten review comment:
getWalletCapabilitiesdoes not validate chain support per EIP-5792 specification.While the function correctly uses the
'0x0'key for all-chain capabilities, EIP-5792 explicitly requires: "Wallets MUST omit chains they do not support." The function currently returns identical capabilities for every requested chain without validating againstsupportedChains. WhenchainIdsare provided by the client, all are echoed back with capabilities regardless of whether the wallet actually supports them. This violates spec compliance and could advertise capabilities for unsupported networks.Filter
requestChainIdsagainstsupportedChains(available insrc/utils/blockchain.ts) before populating the response.⛔ Skipped due to learnings
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.src/types/index.ts (1)
16-22: LGTM!The optional
batchNamefield addition is backward-compatible and correctly typed. It aligns with the new WalletConnect batch transaction handling that uses this field for EIP-5792 batch identification.src/utils/walletConnect.ts (1)
13-15: LGTM!The new EIP-5792 wallet method constants are correctly defined and follow the existing naming pattern. These align with the WalletConnect protocol specifications for batch transaction handling.
src/components/BottomMenuModal/SendModal/SendModalTokensTabView.tsx (3)
1073-1074: LGTM!The
batchNameextraction with fallback tobatch-${batch.chainId}is consistent and correctly uses the optional field from the payload. This enables proper batch identification for EIP-5792 compliance while maintaining backward compatibility.
1091-1092: Batch name extraction is consistent with creation logic.Good consistency between how batch names are assigned during creation (line 1074) and how they're used for estimation (line 1092). This ensures the correct batches are estimated and sent.
2354-2354: UI condition correctly hides asset selection for payload flows.The change from
{!isPayloadTransaction && ...}to{isPayloadTransaction || isPayloadBatches ? null : ...}correctly hides the asset selection UI for both single transaction and batch payloads from WalletConnect. This prevents user confusion when the transaction details are already determined by the dApp.src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx (2)
2-7: Imports align with new hook usage
useStateanduseAccountare correctly imported and used (isAddCashLoading,isConnected). No issues here.
29-30: Destructuring onlywalletAddressfromuseTransactionKitis appropriateDropping
kithere is fine since this component only needswalletAddressfor the on-ramp flow and button disabling; wallet mode checks can remain centralized elsewhere (e.g., via verification hooks in container-level providers).Based on learnings, this matches how Transaction Kit is provided and consumed in this codebase.
src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
Outdated
Show resolved
Hide resolved
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Just a few suggestions and some stuff from CR but none of it is a showstopper 👍
src/services/walletConnect.ts
Outdated
| const serializeBigInts = (obj: any): any => { | ||
| if (obj === null || obj === undefined) { | ||
| return obj; | ||
| } | ||
| if (typeof obj === 'bigint') { | ||
| return obj.toString(); | ||
| } | ||
| if (Array.isArray(obj)) { | ||
| return obj.map(serializeBigInts); | ||
| } | ||
| if (typeof obj === 'object') { | ||
| const serialized: any = {}; | ||
| for (const [key, value] of Object.entries(obj)) { | ||
| serialized[key] = serializeBigInts(value); | ||
| } | ||
| return serialized; | ||
| } | ||
| return obj; | ||
| }; |
There was a problem hiding this comment.
This feels like it could be a utility
There was a problem hiding this comment.
Agreed, I have moved this now to a utility file
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/walletConnect.ts (1)
96-144: Add null check foruserOpStatusto prevent runtime errors.The
getUserOperationStatusfunction can returnundefined, but line 103 accessesuserOpStatus.transactionwithout checking. This will throw a TypeError if the userOp status is not yet available.Apply this diff to add the null check:
if (userOpHash) { try { const userOpStatus = await getUserOperationStatus( chainIdNumber, userOpHash ); + if (!userOpStatus) { + // userOp not yet indexed, return pending + return { + version: '1.0', + id: batchId, + atomic: true, + status: 'PENDING', + }; + } + const finalTxHash = userOpStatus.transaction || txHash;
🧹 Nitpick comments (1)
src/services/walletConnect.ts (1)
54-58: Consider adding cleanup for in-memory batch mapping.The
batchIdToTxDataMapstores batch ID mappings indefinitely, which could lead to memory growth over time. Consider implementing a cleanup mechanism to remove old entries (e.g., after a certain time period or when status reaches a terminal state).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/apps/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsx(2 hunks)src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx(3 hunks)src/services/walletConnect.ts(7 hunks)src/utils/common.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/apps/pillarx-app/components/WalletPortfolioButtons/WalletPortfolioButtons.tsx
- src/apps/pillarx-app/components/PortfolioOverview/PortfolioOverview.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/services/walletConnect.ts
🧬 Code graph analysis (1)
src/services/walletConnect.ts (4)
src/apps/deposit/utils/blockchain.tsx (1)
getNetworkViem(71-94)src/services/userOpStatus.ts (1)
getUserOperationStatus(3-44)src/utils/walletConnect.ts (3)
WALLET_GET_CAPABILITIES(13-13)WALLET_SEND_CALLS(14-14)WALLET_GET_CALLS_STATUS(15-15)src/utils/common.ts (1)
serializeBigInts(114-134)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (6)
src/utils/common.ts (1)
112-134: LGTM! BigInt serialization utility is well-implemented.The recursive function correctly handles the conversion of BigInt values to strings for JSON serialization, which is necessary for WalletConnect responses.
src/services/walletConnect.ts (5)
200-237: LGTM! Wallet capabilities helper correctly implements EIP-5792.The function properly returns per-chain capabilities and differentiates between modular and delegatedEoa wallet modes.
1161-1169: LGTM! Optional chaining is now complete.The wallet mode check now properly uses optional chaining on all method calls, preventing runtime errors if
kitis undefined.
1440-1448: LGTM! Wallet capabilities request handling is correct.Properly delegates to the helper function with appropriate parameters.
1614-1624: LGTM! Calls status request handling is correct.Properly validates parameters and delegates to the helper function.
1626-1638: LGTM! Response serialization and error handling are well-implemented.The BigInt serialization ensures JSON compatibility, and the nested try-catch provides proper error handling for the response sending.
| if (sendCallsParams.calls.length === 1) { | ||
| const singleCall = sendCallsParams.calls[0]; | ||
|
|
||
| // Validate the single call | ||
| if (!singleCall.to || !singleCall.data) { | ||
| console.warn( | ||
| 'WalletConnect wallet_sendCalls: Invalid single call', | ||
| singleCall | ||
| ); | ||
| // eslint-disable-next-line quotes | ||
| throw new Error("Invalid call: missing 'to' or 'data'"); | ||
| } | ||
|
|
||
| // Create a transaction object similar to eth_sendTransaction | ||
| const transaction = { | ||
| to: singleCall.to, | ||
| data: singleCall.data, | ||
| value: singleCall.value || '0x0', | ||
| }; | ||
|
|
||
| const isApprovalTransaction = | ||
| !!transaction.data?.startsWith('0x095ea7b3'); | ||
|
|
||
| if (isApprovalTransaction) { | ||
| const approvalRequest = { | ||
| title: 'WalletConnect Approval Request', | ||
| description: `${dAppName} is requesting approval for a contract.`, | ||
| transaction: { | ||
| to: checksumAddress(transaction.to as `0x${string}`), | ||
| data: transaction.data, | ||
| chainId: chainIdNumber, | ||
| }, | ||
| }; | ||
|
|
||
| showTransactionConfirmation(approvalRequest); | ||
| setWalletConnectPayload(approvalRequest); | ||
| } else { | ||
| const transactionRequest = { | ||
| title: 'WalletConnect Transaction Request', | ||
| description: `${dAppName} wants to send a transaction`, | ||
| transaction: { | ||
| to: checksumAddress(transaction.to as `0x${string}`), | ||
| value: transaction.value | ||
| ? formatEther( | ||
| hexToBigInt(transaction.value as `0x${string}`) | ||
| ) | ||
| : '0', | ||
| data: transaction.data, | ||
| chainId: chainIdNumber, | ||
| }, | ||
| }; | ||
|
|
||
| showTransactionConfirmation(transactionRequest); | ||
| setWalletConnectPayload(transactionRequest); | ||
| } | ||
|
|
||
| // Wait for transaction hash | ||
| const txHash = await getTransactionHash(); | ||
| if (txHash) { | ||
| // Store the mapping for wallet_getCallsStatus | ||
| batchIdToTxDataMap.set(batchId, { txHash }); | ||
| } | ||
|
|
||
| // Returns batch ID for EIP-5792 compliance | ||
| requestResponse = batchId; | ||
| } | ||
|
|
||
| // Multi-transaction batch flow | ||
| // Prepare the transaction data in the batch | ||
| // The batch will be created when user confirms in the SendModal | ||
| const transactionData = sendCallsParams.calls.map((call, i) => { | ||
| if (!call.to || !call.data) { | ||
| throw new Error( | ||
| `Invalid call at index ${i}: missing 'to' or 'data'` | ||
| ); | ||
| } | ||
|
|
||
| return { | ||
| to: checksumAddress(call.to as `0x${string}`), | ||
| value: call.value | ||
| ? formatEther(hexToBigInt(call.value as `0x${string}`)) | ||
| : '0', | ||
| data: call.data as `0x${string}`, | ||
| chainId: callsChainId, | ||
| }; | ||
| }); | ||
|
|
||
| // Create confirmation payload | ||
| const batchRequest = { | ||
| title: 'WalletConnect Batch Transaction Request', | ||
| description: `${dAppName} wants to execute ${sendCallsParams.calls.length} transaction${sendCallsParams.calls.length > 1 ? 's' : ''}`, | ||
| batches: [ | ||
| { | ||
| chainId: callsChainId, | ||
| transactions: transactionData, | ||
| batchName: batchId, | ||
| }, | ||
| ], | ||
| onSent: async (userOpHashes: string[]) => { | ||
| // Get transaction hash from userOp hash and store it with the batch ID | ||
| if (userOpHashes && userOpHashes.length > 0) { | ||
| const userOpHash = userOpHashes[0]; | ||
| const txHash = await kit.getTransactionHash( | ||
| userOpHash, | ||
| callsChainId | ||
| ); | ||
| if (txHash) { | ||
| setWalletConnectTxHash(txHash); | ||
| // Store mapping of batchId -> {txHash, userOpHash} for proper wallet_getCallsStatus lookup | ||
| batchIdToTxDataMap.set(batchId, { txHash, userOpHash }); | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| showTransactionConfirmation(batchRequest); | ||
| setWalletConnectPayload(batchRequest); | ||
|
|
||
| // Returns batch ID for EIP-5792 compliance | ||
| requestResponse = batchId; | ||
| } |
There was a problem hiding this comment.
Fix control flow: single-call path executes multi-call code.
The single-call handling (lines 1492-1557) doesn't prevent execution of the multi-call code (lines 1559-1612). This causes:
- Double confirmation dialogs shown to the user
requestResponseset twice (lines 1556 and 1611)- Unnecessary processing of the same call as a batch
The previous issue was an early return that prevented sending the response. Removing the return fixed that, but now both code paths execute.
Wrap the multi-call logic in an else block:
// Returns batch ID for EIP-5792 compliance
requestResponse = batchId;
- }
+ } else {
+ // Multi-transaction batch flow
+ // Prepare the transaction data in the batch
+ // The batch will be created when user confirms in the SendModal
+ const transactionData = sendCallsParams.calls.map((call, i) => {
- // Multi-transaction batch flow
- // Prepare the transaction data in the batch
- // The batch will be created when user confirms in the SendModal
- const transactionData = sendCallsParams.calls.map((call, i) => {Then close the else block after line 1611:
// Returns batch ID for EIP-5792 compliance
requestResponse = batchId;
+ }
}🤖 Prompt for AI Agents
In src/services/walletConnect.ts around lines 1492 to 1612, the single-call
branch does not prevent execution of the multi-call block so both paths run
(double dialogs and requestResponse overwritten). Fix by wrapping the multi-call
logic (currently starting just after the single-call block, lines ~1559-1612) in
an else clause corresponding to if (sendCallsParams.calls.length === 1) { ... }
and close the else after the multi-call code (after line 1611), ensuring the
single-call branch and multi-call branch are mutually exclusive and no duplicate
requestResponse assignments remain.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.