Conversation
🦋 Changeset detectedLatest commit: f720e79 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR adds chainId forwarding support to the account client and improves transaction handling. It includes two changeset files documenting patch releases for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #922 +/- ##
==========================================
- Coverage 72.21% 71.55% -0.66%
==========================================
Files 227 227
Lines 8388 8222 -166
Branches 2699 2628 -71
==========================================
- Hits 6057 5883 -174
- Misses 2109 2110 +1
- Partials 222 229 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the account client to forward the chain ID in wallet_sendCalls and implements automatic chain switching when sending transactions. It also disables retries in the custom transport. A potential issue was identified where swallowing errors during the chain-revert process in the finally block could leave the user's wallet on an incorrect network, leading to inconsistent application state.
| }); | ||
| return { id: concat([hash, numberToHex(requestedChainId, { size: 32 }), TX_MAGIC_ID]) }; | ||
| } finally { | ||
| await switchChain(ownerConfig, { chainId: chain.id }).catch(reportError); |
There was a problem hiding this comment.
The error handling for switching the chain back might leave the application in an inconsistent state. If switchChain fails, the error is reported but then swallowed. This means the user's wallet could be left on a different network than the application expects, which could cause subsequent operations to fail or be executed on the wrong chain. Consider allowing the error to propagate by removing the .catch(), which would prevent the application from continuing in a potentially incorrect state.
await switchChain(ownerConfig, { chainId: chain.id });![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f720e798d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }); | ||
| return { id: concat([hash, numberToHex(requestedChainId, { size: 32 }), TX_MAGIC_ID]) }; | ||
| } finally { | ||
| await switchChain(ownerConfig, { chainId: chain.id }).catch(reportError); |
There was a problem hiding this comment.
The fallback path for wallet_sendCalls switches to requestedChainId and then always switches back to chain.id, which overwrites the user/dapp's current network selection whenever they requested a different chain. In flows where subsequent RPCs omit chainId (or rely on the connector’s active chain), this can silently route follow-up actions to the wrong network. Capture the pre-call chain from the current connection and restore that instead of hard-coding chain.id.
Useful? React with 👍 / 👎.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| await switchChain(ownerConfig, { chainId: chain.id }).catch(reportError); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: If the final switchChain call fails in the finally block, the error is silently caught, leaving the wallet in an inconsistent state on the wrong chain.
Severity: MEDIUM
Suggested Fix
The finally block should handle the failure of the chain switch restoration more robustly. Instead of silently catching the error with .catch(reportError), consider re-throwing the error or otherwise ensuring the caller is notified about the failed restoration to prevent state inconsistency. Adding retry logic could also be an option.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/utils/accountClient.ts#L215-L217
Potential issue: In the `wallet_sendCalls` function, the code switches to a
`requestedChainId` to send a transaction and attempts to switch back to the original
`chain.id` in a `finally` block. If the final `switchChain` operation fails, the error
is silently caught and reported. This leaves the wallet on the `requestedChainId`,
creating an inconsistent state for subsequent operations which may then execute on the
wrong chain without any indication to the user or caller.
Did we get this right? 👍 / 👎 to inform future reviews.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| try { | ||
| return await sendCalls(ownerConfig, { | ||
| id, | ||
| chainId: chainId ? hexToNumber(chainId) : chain.id, |
There was a problem hiding this comment.
🟡 Duplicate expression violates AGENTS.md extraction rule ("two or more uses earn a name")
The expression chainId ? hexToNumber(chainId) : chain.id is computed twice: once inline at src/utils/accountClient.ts:189 and again as the extracted variable requestedChainId at src/utils/accountClient.ts:205. Per the AGENTS.md extraction rule — "two or more uses earn a name: the threshold for extraction is a second call site" — this expression should be defined once before the try block and reused in both the sendCalls call and the catch block.
Prompt for agents
In src/utils/accountClient.ts, hoist the `requestedChainId` variable definition from line 205 (inside the catch block) to before the try block (around line 186), so the expression `chainId ? hexToNumber(chainId) : chain.id` is only written once. Then replace the inline expression at line 189 (`chainId: chainId ? hexToNumber(chainId) : chain.id`) with `chainId: requestedChainId`, and remove the duplicate `const requestedChainId = ...` at line 205.
Was this helpful? React with 👍 or 👎 to provide feedback.
| await switchChain(ownerConfig, { chainId: requestedChainId }); | ||
| try { | ||
| const hash = await sendTransaction(ownerConfig, { | ||
| to: accountAddress, | ||
| data: encodeFunctionData(execute), | ||
| chainId: requestedChainId, | ||
| }); | ||
| return { id: concat([hash, numberToHex(requestedChainId, { size: 32 }), TX_MAGIC_ID]) }; | ||
| } finally { | ||
| await switchChain(ownerConfig, { chainId: chain.id }).catch(reportError); | ||
| } |
There was a problem hiding this comment.
🚩 Race condition risk in chain switching pattern
The catch block at src/utils/accountClient.ts:206-216 implements a switch-chain → send-transaction → switch-back pattern on the shared ownerConfig. If multiple concurrent wallet_sendCalls requests hit this fallback path simultaneously, they could interleave switchChain calls, causing one request to send on another's chain. In practice this is mitigated by: (1) this is already a fallback path (only reached when sendCalls fails), (2) sendTransaction includes an explicit chainId parameter which wagmi should validate, and (3) UI flows likely prevent concurrent wallet operations. However, the theoretical race exists and could matter if this code is ever called concurrently.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (queryClient.getQueryData<AuthMethod>(["method"]) === "webauthn") { | ||
| const { hash } = await client.sendUserOperation({ |
There was a problem hiding this comment.
🚩 Webauthn path does not forward the requested chainId
The wallet_sendCalls handler now extracts chainId from params (src/utils/accountClient.ts:167-169) and forwards it in the SIWE/owner paths. However, the webauthn path at src/utils/accountClient.ts:174-178 ignores the requested chainId entirely — it always sends the user operation on chain and encodes chain.id in the magic ID. This is internally consistent (the smart account client is hardwired to chain), but if a caller requests a different chain, the operation silently proceeds on the wrong chain. This appears intentional since the webauthn user operation infrastructure only supports a single chain, but a validation like if (chainId && hexToNumber(chainId) !== chain.id) throw new Error("unsupported chain") would prevent silent misrouting.
(Refers to lines 174-178)
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/accountClient.ts (1)
174-179:⚠️ Potential issue | 🟠 MajorDon't silently ignore
chainIdfor WebAuthn calls.This branch still submits through the client bound to
chainand always encodeschain.idin the returned id, so a cross-chainwallet_sendCallsrequest can appear successful on the wrong network. Either plumbchainIdthrough this path or fail fast when it differs fromchain.id.💡 minimal fail-fast fix
if (queryClient.getQueryData<AuthMethod>(["method"]) === "webauthn") { + if (chainId && hexToNumber(chainId) !== chain.id) throw new Error("unsupported chain"); const { hash } = await client.sendUserOperation({ uo: calls.map(({ to, data = "0x", value }) => ({ from: accountAddress, target: to, data, value })), }); return { id: concat([hash, numberToHex(chain.id, { size: 32 }), UO_MAGIC_ID]) }; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1de07854-30d8-47b5-a8f1-2e4e084a1fb1
📒 Files selected for processing (3)
.changeset/cool-icons-grow.md.changeset/fuzzy-adults-tease.mdsrc/utils/accountClient.ts
| transport: custom( | ||
| // prettier-ignore | ||
| { | ||
| async request({ method, params }) { |
There was a problem hiding this comment.
Remove the prettier-ignore suppression.
This repo leaves formatting to Prettier and does not allow this suppression in source files.
As per coding guidelines "Follow ESLint, Prettier, and Solhint rules strictly; do not argue with the linter" and "Use only static analysis annotations (@ts-expect-error, eslint-disable, solhint-disable, slither-disable, cspell:ignore) and TODO/HACK/FIXME markers".
| const requestedChainId = chainId ? hexToNumber(chainId) : chain.id; | ||
| await switchChain(ownerConfig, { chainId: requestedChainId }); | ||
| try { | ||
| const hash = await sendTransaction(ownerConfig, { | ||
| to: accountAddress, | ||
| data: encodeFunctionData(execute), | ||
| chainId: requestedChainId, | ||
| }); | ||
| return { id: concat([hash, numberToHex(requestedChainId, { size: 32 }), TX_MAGIC_ID]) }; | ||
| } finally { | ||
| await switchChain(ownerConfig, { chainId: chain.id }).catch(reportError); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
nl -ba src/utils/wagmi/owner.ts | sed -n '1,40p'
printf '\n---\n'
nl -ba src/utils/accountClient.ts | sed -n '200,220p'Repository: exactly/exa
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check wagmi/owner.ts
echo "=== src/utils/wagmi/owner.ts (first 40 lines) ==="
cat -n src/utils/wagmi/owner.ts | head -40
echo ""
echo "=== src/utils/accountClient.ts (lines 200-220) ==="
cat -n src/utils/accountClient.ts | sed -n '200,220p'Repository: exactly/exa
Length of output: 2972
🏁 Script executed:
#!/bin/bash
# Check imports and broader context in accountClient.ts
echo "=== Imports in accountClient.ts ==="
head -30 src/utils/accountClient.ts
echo ""
echo "=== Full context around lines 190-220 (entire fallback block) ==="
cat -n src/utils/accountClient.ts | sed -n '190,220p'
echo ""
echo "=== Search for ownerConfig usage ==="
rg "ownerConfig" src/utils/accountClient.ts -B 2 -A 2 | head -40Repository: exactly/exa
Length of output: 4370
🏁 Script executed:
#!/bin/bash
# Find the function containing this code block
echo "=== Context to find function signature (lines 150-210) ==="
cat -n src/utils/accountClient.ts | sed -n '150,210p'
echo ""
echo "=== Check for concurrent patterns (looking for request handlers) ==="
rg "request|handler|async.*\(.*\)" src/utils/accountClient.ts | head -20Repository: exactly/exa
Length of output: 3794
🏁 Script executed:
#!/bin/bash
# Check for any queue/lock mechanisms or serialization
echo "=== Check for serialization or queuing patterns ==="
rg "Queue|queue|Mutex|mutex|lock|Lock|serialize|Serialize" src/utils/accountClient.ts
echo ""
echo "=== Check if ownerConfig has any built-in synchronization ==="
rg "switchChain|sendTransaction" src/utils/wagmi/ -A 2 -B 2 | head -30Repository: exactly/exa
Length of output: 169
Don't switch a shared wagmi config inside this fallback.
src/utils/wagmi/owner.ts exports a singleton config (default export, imported as ownerConfig). The fallback handler at lines 205–215 is inside an async request({ method, params }) transport handler that can be invoked concurrently by multiple consumers. Concurrent calls race on the shared state: one request switches to chainId X, another to chainId Y, and their finally blocks restore to a hard-coded chain.id, leaving the shared client on the wrong chain. Even single calls corrupt state—if a user is already on a different chain, this block switches them away and restores to the imported constant, not their actual previous chain.
Capture the previous chain before switching, restore that exact value, serialize this section with a lock, or move to an isolated per-request client.
| const { to, data = "0x", value = 0n } = params[0] as TransactionRequest; | ||
| const { id } = await sendCalls(ownerConfig, { | ||
| chainId: chain.id, |
There was a problem hiding this comment.
Don't pin eth_sendTransaction to the default chain.
This branch drops the request's chainId and forces sendCalls onto chain.id, so cross-chain eth_sendTransaction requests still execute on the default network. Use the caller's chain when present, or reject mismatches explicitly.
💡 compact fix
- const { to, data = "0x", value = 0n } = params[0] as TransactionRequest;
+ const { to, chainId, data = "0x", value = 0n } = params[0] as TransactionRequest;
const { id } = await sendCalls(ownerConfig, {
- chainId: chain.id,
+ chainId: chainId ?? chain.id,
calls: [1 participant
Summary by CodeRabbit