Conversation
WalkthroughThe changes remove the Changes
Sequence DiagramsequenceDiagram
participant App as Main Component
participant WC as Viem WalletClient
participant EP as EIP-1193 Provider
participant Auth as Authorized Component
participant Kit as EtherspotTransactionKitProvider
App->>WC: Create walletClient (read-only, http transport)
App->>EP: createPrivateKeyProvider(walletClient)
EP-->>App: EIP-1193 provider with custom transport
App->>Auth: Pass {provider, chainId}
Auth->>Kit: Pass {provider, chainId, bundlerApiKey}
Kit-->>Auth: Transaction kit initialized
rect rgb(200, 220, 255)
Note over EP,Kit: Request-response cycle<br/>(e.g., transaction signing)
Kit->>EP: eth_sendTransaction
EP->>WC: walletClient.sendTransaction()
WC-->>EP: tx hash
EP-->>Kit: Signed tx hash
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve: (1) straightforward prop removal from a component, (2) provider setup refactoring with new control flow, and (3) a new utility function implementing multiple EIP-1193 RPC methods with associated logic and error handling. The changes are cohesive around private-key provider setup but span multiple files with distinct concerns. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: |
67fb8a3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b3eb6270.x-e62.pages.dev |
| Branch Preview URL: | https://fix-pk-wc-connector-priority.x-e62.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/utils/privateKeyProvider.ts (2)
86-88: Replaceas neverwith proper typing.The
as nevertype assertion bypasses TypeScript's type checking and could hide legitimate type mismatches. Consider properly typingtransactionParamsto match Viem'ssendTransactionexpectations or using a more specific type assertion.- const hash = await walletClient.sendTransaction( - transactionParams as never - ); + // Import proper types from viem + const hash = await walletClient.sendTransaction( + transactionParams as Parameters<typeof walletClient.sendTransaction>[0] + );Or better yet, build the params object with the correct type from the start by importing Viem's transaction types.
195-198: Reduce type assertions for read-only method delegation.The multiple type assertions (
as string,as unknown[],as never) suggest type mismatches. Sincemethodandparamsare already correctly typed from the function signature, consider refining the types or the request call to avoid these assertions.const result = await walletClient.request({ - method: method as string, - params: params as unknown[], - } as never); + method, + params, + });If TypeScript still complains, investigate the underlying type mismatch rather than suppressing it with assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/containers/Authorized.tsx(1 hunks)src/containers/Main.tsx(5 hunks)src/utils/privateKeyProvider.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/containers/Main.tsx (2)
src/utils/privateKeyProvider.ts (1)
createPrivateKeyProvider(12-238)src/containers/Authorized.tsx (1)
Authorized(30-159)
🔇 Additional comments (4)
src/containers/Main.tsx (2)
267-282: Verify the need for multiple wrapper layers in provider setup.The current implementation creates a wallet client (
walletClientForReads), wraps it in an EIP-1193 provider (eip1193Provider), then wraps that in another wallet client (newProvider). This creates a call chain:newProvider→eip1193Provider→walletClientForReads→ RPC.Questions to verify:
- Does EtherspotTransactionKit specifically require an EIP-1193 provider interface (not just a Viem WalletClient)?
- Could
createPrivateKeyProviderbe simplified to not require a wallet client as input, instead creating the transport internally?- Is the custom transport wrapper necessary, or could Etherspot use the
eip1193Providerdirectly?If the layering is required by Etherspot's API, consider adding a comment explaining why. If not, simplifying to a single wallet client with http transport would reduce complexity:
// If EtherspotTransactionKit can accept a standard Viem WalletClient: const newProvider = createWalletClient({ account, chain: getNetworkViem(walletChainId), transport: http(), });
685-685: LGTM! Clean separation of concerns.Removing the
privateKeyprop fromAuthorizedimproves encapsulation by keeping the private key handling within the provider layer. The component now only needs the configured provider and chain ID.src/containers/Authorized.tsx (2)
30-36: LGTM! Props interface correctly updated.The removal of the
privateKeyprop simplifies the component's interface and aligns with the new provider-based architecture where private key handling is encapsulated in the provider layer.
119-119: LGTM! Dependency array correctly updated.The
useMemodependency array is correctly updated to reflect the removedprivateKeyprop. The config now correctly memoizes based only onproviderandchainId.
| // Create a wallet client with http transport for read operations | ||
| const walletClientForReads = createWalletClient({ | ||
| account, | ||
| chain: getNetworkViem(walletChainId), | ||
| transport: http(), | ||
| }); | ||
|
|
||
| // Wrap it in an EIP-1193 provider for signing operations | ||
| const eip1193Provider = createPrivateKeyProvider(walletClientForReads); | ||
|
|
||
| // Create the final provider with custom transport using the EIP-1193 wrapper | ||
| const newProvider = createWalletClient({ | ||
| account, | ||
| chain: getNetworkViem(walletChainId), | ||
| transport: custom(eip1193Provider), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify the provider architecture to avoid redundant wrapper layers.
The current setup creates multiple wrapper layers:
walletClientForReads(wallet client with http transport)eip1193Provider(wraps walletClientForReads with EIP-1193 interface)newProvider(wallet client with custom(eip1193Provider) transport)
This creates unnecessary indirection where read operations go through: newProvider → eip1193Provider → walletClientForReads → http transport.
Consider simplifying to a single wallet client:
const account = privateKeyToAccount(privateKey as `0x${string}`);
const walletChainId = 1; // default chain id is 1 (mainnet)
- // Create a wallet client with http transport for read operations
- const walletClientForReads = createWalletClient({
- account,
- chain: getNetworkViem(walletChainId),
- transport: http(),
- });
-
- // Wrap it in an EIP-1193 provider for signing operations
- const eip1193Provider = createPrivateKeyProvider(walletClientForReads);
-
- // Create the final provider with custom transport using the EIP-1193 wrapper
- const newProvider = createWalletClient({
+ // Create wallet client - viem wallet clients are already EIP-1193 compatible
+ const newProvider = createWalletClient({
account,
chain: getNetworkViem(walletChainId),
- transport: custom(eip1193Provider),
+ transport: http(),
});Alternatively, if EtherspotTransactionKit specifically requires the EIP-1193 wrapper, verify why and document the requirement. The current architecture may be introducing unnecessary complexity.
📝 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 a wallet client with http transport for read operations | |
| const walletClientForReads = createWalletClient({ | |
| account, | |
| chain: getNetworkViem(walletChainId), | |
| transport: http(), | |
| }); | |
| // Wrap it in an EIP-1193 provider for signing operations | |
| const eip1193Provider = createPrivateKeyProvider(walletClientForReads); | |
| // Create the final provider with custom transport using the EIP-1193 wrapper | |
| const newProvider = createWalletClient({ | |
| account, | |
| chain: getNetworkViem(walletChainId), | |
| transport: custom(eip1193Provider), | |
| }); | |
| // Create wallet client - viem wallet clients are already EIP-1193 compatible | |
| const newProvider = createWalletClient({ | |
| account, | |
| chain: getNetworkViem(walletChainId), | |
| transport: http(), | |
| }); |
🤖 Prompt for AI Agents
In src/containers/Main.tsx around lines 267 to 282, the code builds three nested
layers (walletClientForReads -> eip1193Provider -> newProvider) causing
redundant indirection; replace this with a single wallet client and only wrap
with an EIP-1193 provider if a downstream consumer explicitly requires it:
either (A) create one wallet client with http() for reads and use that directly,
or (B) if EtherspotTransactionKit needs EIP-1193, construct the EIP-1193
provider once from the account and pass it directly to
createWalletClient(custom(...)) without creating an intermediate http-based
wallet client; remove the unused walletClientForReads, update callers to use the
single provider, and add a short comment documenting why an EIP-1193 wrapper is
required if you choose option B.
| case 'eth_accounts': | ||
| case 'eth_requestAccounts': | ||
| return [walletClient.account?.address]; | ||
|
|
||
| case 'eth_chainId': | ||
| return `0x${walletClient.chain?.id.toString(16)}`; |
There was a problem hiding this comment.
Handle undefined account/chain gracefully.
EIP-1193 requires eth_accounts to return an array of addresses (not [undefined]) and eth_chainId to return a valid hex string. Currently:
- Line 30 returns
[undefined]ifwalletClient.accountis undefined - Line 33 returns
"0xundefined"ifwalletClient.chainis undefined
Apply this diff:
case 'eth_accounts':
case 'eth_requestAccounts':
- return [walletClient.account?.address];
+ if (!walletClient.account?.address) {
+ return [];
+ }
+ return [walletClient.account.address];
case 'eth_chainId':
- return `0x${walletClient.chain?.id.toString(16)}`;
+ if (!walletClient.chain?.id) {
+ throw new Error('Chain ID not available');
+ }
+ return `0x${walletClient.chain.id.toString(16)}`;🤖 Prompt for AI Agents
In src/utils/privateKeyProvider.ts around lines 28–33, the handler currently
returns [undefined] for eth_accounts and "0xundefined" for eth_chainId when
walletClient.account or walletClient.chain is missing; change eth_accounts to
return an empty array when no account is present (e.g.
walletClient.account?.address ? [address] : []) and change eth_chainId to only
build and return the hex string when walletClient.chain exists (e.g.
walletClient.chain ? `0x${walletClient.chain.id.toString(16)}` : undefined) so
callers get a proper empty list or undefined instead of invalid values.
| const transactionParams: Record<string, unknown> = { | ||
| account: walletClient.account, | ||
| to: transaction.to, | ||
| value: transaction.value | ||
| ? BigInt(transaction.value as string) | ||
| : undefined, | ||
| data: transaction.data, | ||
| gas: transaction.gas | ||
| ? BigInt(transaction.gas as string) | ||
| : undefined, | ||
| nonce: transaction.nonce ? Number(transaction.nonce) : undefined, | ||
| }; | ||
|
|
||
| // Add gas pricing based on transaction type | ||
| if (transaction.gasPrice) { | ||
| transactionParams.gasPrice = BigInt( | ||
| transaction.gasPrice as string | ||
| ); | ||
| } else if ( | ||
| transaction.maxFeePerGas && | ||
| transaction.maxPriorityFeePerGas | ||
| ) { | ||
| transactionParams.maxFeePerGas = BigInt( | ||
| transaction.maxFeePerGas as string | ||
| ); | ||
| transactionParams.maxPriorityFeePerGas = BigInt( | ||
| transaction.maxPriorityFeePerGas as string | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate transaction parameters before type conversion.
The BigInt and Number conversions (lines 59-66, 71-83) will throw if the input values are not valid numbers or numeric strings. Consider validating the transaction parameters or wrapping conversions in try-catch blocks to provide clearer error messages.
Example defensive approach for value conversion:
const transactionParams: Record<string, unknown> = {
account: walletClient.account,
to: transaction.to,
- value: transaction.value
- ? BigInt(transaction.value as string)
- : undefined,
+ value: transaction.value ? (() => {
+ try {
+ return BigInt(transaction.value as string);
+ } catch (error) {
+ throw new Error(`Invalid transaction value: ${transaction.value}`);
+ }
+ })() : undefined,
data: transaction.data,
// ... similar for gas, nonce, gasPrice, etc.Apply similar validation for gas, nonce, gasPrice, maxFeePerGas, and maxPriorityFeePerGas.
Committable suggestion skipped: line range outside the PR's diff.
| const signature = await walletClient.signMessage({ | ||
| account: walletClient.account, | ||
| message: { raw: hexToBytes(message as Hex) }, | ||
| }); |
There was a problem hiding this comment.
Validate hex format before conversion.
hexToBytes will throw if message is not a valid hex string. Consider validating the format or wrapping in a try-catch to provide a clearer error message.
if (!walletClient.account) {
throw new Error('Account not available');
}
+ let messageBytes;
+ try {
+ messageBytes = hexToBytes(message as Hex);
+ } catch (error) {
+ throw new Error(`Invalid hex message format: ${message}`);
+ }
const signature = await walletClient.signMessage({
account: walletClient.account,
- message: { raw: hexToBytes(message as Hex) },
+ message: { raw: messageBytes },
});Apply the same pattern to line 142 in the eth_sign case.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils/privateKeyProvider.ts around lines 116-119, the code calls
hexToBytes(message) without validating the message format so it will throw on
invalid hex; wrap the conversion in a try-catch (or pre-validate with a hex
regex) to detect invalid hex and throw or log a clearer, descriptive error
(e.g., "invalid hex message for signMessage") rather than letting hexToBytes
blow up; apply the same validation/wrapping pattern to the eth_sign branch at
line 142 to ensure both paths validate the hex before conversion and provide
consistent error handling.
| case 'eth_sign': { | ||
| const [address, message] = params || []; | ||
| if (!message || !address) { | ||
| throw new Error('Message or address parameter missing'); | ||
| } | ||
|
|
||
| Sentry.addBreadcrumb({ | ||
| category: 'private_key_provider', | ||
| message: 'Signing message (eth_sign) with private key', | ||
| level: 'info', | ||
| }); | ||
|
|
||
| // Sign the message using the wallet client | ||
| if (!walletClient.account) { | ||
| throw new Error('Account not available'); | ||
| } | ||
| const signature = await walletClient.signMessage({ | ||
| account: walletClient.account, | ||
| message: { raw: hexToBytes(message as Hex) }, | ||
| }); | ||
|
|
||
| return signature; | ||
| } |
There was a problem hiding this comment.
Security: eth_sign is deprecated and dangerous.
The eth_sign method allows blind signing of arbitrary data and has been deprecated by MetaMask and other wallets due to phishing risks. Users can be tricked into signing malicious transactions. Consider either removing support for this method or adding prominent warnings in logs.
See: MetaMask's eth_sign deprecation
If you must support it, add a clear warning:
case 'eth_sign': {
+ Sentry.captureMessage('eth_sign called - deprecated and insecure method', {
+ level: 'warning',
+ tags: { component: 'private_key_provider', security: 'deprecated_method' },
+ });
+
const [address, message] = params || [];📝 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.
| case 'eth_sign': { | |
| const [address, message] = params || []; | |
| if (!message || !address) { | |
| throw new Error('Message or address parameter missing'); | |
| } | |
| Sentry.addBreadcrumb({ | |
| category: 'private_key_provider', | |
| message: 'Signing message (eth_sign) with private key', | |
| level: 'info', | |
| }); | |
| // Sign the message using the wallet client | |
| if (!walletClient.account) { | |
| throw new Error('Account not available'); | |
| } | |
| const signature = await walletClient.signMessage({ | |
| account: walletClient.account, | |
| message: { raw: hexToBytes(message as Hex) }, | |
| }); | |
| return signature; | |
| } | |
| case 'eth_sign': { | |
| Sentry.captureMessage('eth_sign called - deprecated and insecure method', { | |
| level: 'warning', | |
| tags: { component: 'private_key_provider', security: 'deprecated_method' }, | |
| }); | |
| const [address, message] = params || []; | |
| if (!message || !address) { | |
| throw new Error('Message or address parameter missing'); | |
| } | |
| Sentry.addBreadcrumb({ | |
| category: 'private_key_provider', | |
| message: 'Signing message (eth_sign) with private key', | |
| level: 'info', | |
| }); | |
| // Sign the message using the wallet client | |
| if (!walletClient.account) { | |
| throw new Error('Account not available'); | |
| } | |
| const signature = await walletClient.signMessage({ | |
| account: walletClient.account, | |
| message: { raw: hexToBytes(message as Hex) }, | |
| }); | |
| return signature; | |
| } |
| const parsedData = | ||
| typeof typedData === 'string' ? JSON.parse(typedData) : typedData; | ||
|
|
||
| // Sign typed data using the wallet client | ||
| if (!walletClient.account) { | ||
| throw new Error('Account not available'); | ||
| } | ||
| const signature = await walletClient.signTypedData({ | ||
| account: walletClient.account, | ||
| domain: parsedData.domain, | ||
| types: parsedData.types, | ||
| primaryType: parsedData.primaryType, | ||
| message: parsedData.message, | ||
| }); |
There was a problem hiding this comment.
Validate JSON parsing and typed data structure.
Line 162's JSON.parse will throw if typedData is not valid JSON. Additionally, the parsed data should be validated to ensure it has the required fields (domain, types, primaryType, message) before passing to signTypedData.
- const parsedData =
- typeof typedData === 'string' ? JSON.parse(typedData) : typedData;
+ let parsedData;
+ try {
+ parsedData = typeof typedData === 'string'
+ ? JSON.parse(typedData)
+ : typedData;
+ } catch (error) {
+ throw new Error('Invalid JSON in typed data parameter');
+ }
+
+ if (!parsedData.domain || !parsedData.types || !parsedData.primaryType || !parsedData.message) {
+ throw new Error('Invalid typed data structure - missing required fields');
+ }
// Sign typed data using the wallet client📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsedData = | |
| typeof typedData === 'string' ? JSON.parse(typedData) : typedData; | |
| // Sign typed data using the wallet client | |
| if (!walletClient.account) { | |
| throw new Error('Account not available'); | |
| } | |
| const signature = await walletClient.signTypedData({ | |
| account: walletClient.account, | |
| domain: parsedData.domain, | |
| types: parsedData.types, | |
| primaryType: parsedData.primaryType, | |
| message: parsedData.message, | |
| }); | |
| let parsedData; | |
| try { | |
| parsedData = typeof typedData === 'string' | |
| ? JSON.parse(typedData) | |
| : typedData; | |
| } catch (error) { | |
| throw new Error('Invalid JSON in typed data parameter'); | |
| } | |
| if (!parsedData.domain || !parsedData.types || !parsedData.primaryType || !parsedData.message) { | |
| throw new Error('Invalid typed data structure - missing required fields'); | |
| } | |
| // Sign typed data using the wallet client | |
| if (!walletClient.account) { | |
| throw new Error('Account not available'); | |
| } | |
| const signature = await walletClient.signTypedData({ | |
| account: walletClient.account, | |
| domain: parsedData.domain, | |
| types: parsedData.types, | |
| primaryType: parsedData.primaryType, | |
| message: parsedData.message, | |
| }); |
🤖 Prompt for AI Agents
In src/utils/privateKeyProvider.ts around lines 161 to 174, the code calls
JSON.parse on typedData and then uses parsedData fields without validation; wrap
JSON.parse in a try/catch to throw a clear error if parsing fails (or accept
non-string safely), then validate that parsedData is an object and contains the
required keys domain, types, primaryType, and message (e.g. typeof checks and
Array/Object checks or a simple type guard), throwing descriptive errors if any
are missing, and only then call walletClient.signTypedData with the validated
fields; keep existing account existence check.
| on: () => provider, | ||
| removeListener: () => provider, | ||
| once: () => provider, |
There was a problem hiding this comment.
Implement proper EIP-1193 event emitter or document limitations.
The current event emitter implementation is a stub that accepts listeners but never invokes them. EIP-1193 providers should emit events like chainChanged, accountsChanged, connect, and disconnect. Libraries like EtherspotTransactionKit may rely on these events for state synchronization.
Options:
- Implement a basic event emitter with proper listener storage and emission
- Document that this provider doesn't support events and verify that EtherspotTransactionKit doesn't require them
- Use a library like
eventemitter3for proper event handling
Example minimal implementation:
const listeners = new Map<string, Set<Function>>();
const provider = {
async request(...) { /* existing */ },
on(event: string, listener: Function) {
if (!listeners.has(event)) {
listeners.set(event, new Set());
}
listeners.get(event)!.add(listener);
return provider;
},
removeListener(event: string, listener: Function) {
listeners.get(event)?.delete(listener);
return provider;
},
once(event: string, listener: Function) {
const onceWrapper = (...args: unknown[]) => {
listener(...args);
provider.removeListener(event, onceWrapper);
};
return provider.on(event, onceWrapper);
},
// Helper to emit events (call when chain/account changes)
emit(event: string, ...args: unknown[]) {
listeners.get(event)?.forEach(listener => listener(...args));
},
};🤖 Prompt for AI Agents
In src/utils/privateKeyProvider.ts around lines 232 to 234, the provider's
on/removeListener/once methods are stubs that never register or invoke listeners
(violating EIP-1193) — replace the stubs with a real event-emitter
implementation: maintain a Map of event -> Set<Function>, implement on(event,
listener) to add listeners and return provider, removeListener(event, listener)
to remove them, once(event, listener) to wrap the listener so it auto-removes
after first call, and add an internal emit(event, ...args) helper to call all
registered listeners; alternatively, if you intentionally don't support events,
add clear inline documentation and throw or return a no-op that documents
unsupported behavior, or import a tested library like eventemitter3 and wire its
on/off/once/emit methods to satisfy EIP-1193 consumers.
RanaBug
left a comment
RanaBug
left a comment
There was a problem hiding this comment.
Just some questions and comments:
- I am not sure to understand why we create an EIP 1193 provider "manually" - is there no helper function from viem we could use for this? Does this not increase the risk of missing details by creating it from scratch?
- The privateKey has been removed and is now passed to a wallet client, the wallet client is not enough to make EIP 7702 work on TransactionKit.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit