🐛 app: custom transport fixes#922
Conversation
🦋 Changeset detectedLatest commit: 381d1b1 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughForwards an optional chainId through account client requests, uses that chainId in generated IDs and sendCalls, disables retries for the bundler transport, and adds chain-switching around a fallback sendTransaction; also adds two changeset files for patch releases of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountClient
participant WalletProvider
participant Blockchain
Client->>AccountClient: wallet_sendCalls({calls, from, chainId?})
AccountClient->>AccountClient: determine requestedChainId = chainId || chain.id
alt sendCalls succeeds
AccountClient->>WalletProvider: sendCalls(..., chainId: requestedChainId)
WalletProvider->>Blockchain: submit calls on requestedChainId
Blockchain-->>WalletProvider: result
WalletProvider-->>AccountClient: result
AccountClient-->>Client: response (id includes requestedChainId)
else sendCalls fails
AccountClient->>AccountClient: switchChain(requestedChainId)
AccountClient->>WalletProvider: sendTransaction({to, data, chainId: requestedChainId})
WalletProvider->>Blockchain: submit tx on requestedChainId
Blockchain-->>WalletProvider: result / error
WalletProvider-->>AccountClient: result / error
AccountClient->>AccountClient: switchChain(originalChainId) (finally)
AccountClient-->>Client: response / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 73.49% 71.91% -1.59%
==========================================
Files 229 229
Lines 8874 8441 -433
Branches 2927 2710 -217
==========================================
- Hits 6522 6070 -452
- Misses 2117 2140 +23
+ Partials 235 231 -4
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
from
f720e79 to
ae0aad2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
from
ae0aad2 to
89535f1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/utils/accountClient.ts (4)
160-163:⚠️ Potential issue | 🟡 MinorRemove the
prettier-ignoresuppression.This repo does not allow formatting suppressions in source files; let Prettier wrap this call normally. 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) andTODO/HACK/FIXMEmarkers; do not use JSDoc, explanatory prose, region markers, or inline labels".
174-179:⚠️ Potential issue | 🟠 MajorDon't encode
requestedChainIdhere unless this branch actually honors it.The WebAuthn path still sends through the default-chain client, and
wallet_getCallsStatuslater polls that same client. IfrequestedChainId !== chain.id, the operation still executes on the default network while the returned id claims otherwise. Either reject mismatches here or create a chain-specific client beforesendUserOperation().💡 minimal guard
const requestedChainId = chainId ? hexToNumber(chainId) : chain.id; if (queryClient.getQueryData<AuthMethod>(["method"]) === "webauthn") { + if (requestedChainId !== chain.id) throw new Error("unsupported chain"); const { hash } = await client.sendUserOperation({
206-215:⚠️ Potential issue | 🔴 CriticalDon't switch the shared
ownerConfiginside this fallback.
src/utils/wagmi/owner.tsexports a singleton config, so concurrent requests can race each other here. Even without concurrency, thefinallyblock clobbers any pre-existing non-default selection because it always hard-resets tochain.id. Use an isolated per-request client, or serialize this section and restore the real prior chain.
239-241:⚠️ Potential issue | 🟠 Major
eth_sendTransactionis still pinned to the default chain.This branch ignores any
chainIdon the incoming transaction and always callssendCalls()withchain.id, so cross-chaineth_sendTransactionrequests still go to the default network. Use the request's chain when present, or reject mismatches before falling back toclient.request().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d93dca5f-a56d-4bf8-a8a6-a55f0ba2227d
📒 Files selected for processing (3)
.changeset/cool-icons-grow.md.changeset/fuzzy-adults-tease.mdsrc/utils/accountClient.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/utils/accountClient.ts (3)
172-177:⚠️ Potential issue | 🟠 MajorReject non-default chains on the WebAuthn path.
client.sendUserOperation()is still tied to the defaultchain, but the returned id is stamped withrequestedChainId. A non-default request can execute on one network and then be polled/reported as another.💡 compact guard
const requestedChainId = chainId ? hexToNumber(chainId) : chain.id; if (queryClient.getQueryData<AuthMethod>(["method"]) === "webauthn") { + if (requestedChainId !== chain.id) throw new Error("unsupported chain"); const { hash } = await client.sendUserOperation({
204-214:⚠️ Potential issue | 🔴 CriticalDon't switch a shared wagmi config inside this fallback.
This still mutates
ownerConfigglobally. Overlapping requests can race on the active chain, and even a single request restores tochain.idinstead of the caller’s actual previous chain. Use an isolated client, or capture/restore the exact previous chain under a lock.
237-239:⚠️ Potential issue | 🟠 MajorDon't hard-code the default chain in
eth_sendTransaction.This branch still ignores
params[0].chainId, so a cross-chain request is silently forced ontochain.id. If this flow is intentionally single-chain, reject mismatches instead of rewriting them.💡 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,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca0632f0-363c-4faf-a10d-f3e234755abf
📒 Files selected for processing (3)
.changeset/cool-icons-grow.md.changeset/fuzzy-adults-tease.mdsrc/utils/accountClient.ts
dieguezguille
force-pushed
the
transport
branch
from
ca87c0e to
6acd9ae
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
2 times, most recently
from
4868ef9 to
eb6d3a6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
from
5c6c745 to
b50b099
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
from
b50b099 to
de51847
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de5184708c
ℹ️ 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".
| dropError: (error) => error instanceof ValiError || classifyError(error).authKnown, | ||
| warnError: (error) => classifyError(error).passkeyWarning, |
There was a problem hiding this comment.
dropError now returns true for every authKnown error, but the global query handler checks dropError before warnError (src/utils/queryClient.ts), so passkeyWarning cases (e.g. biometrics-required/pending-passkey conditions) are dropped and never reach warning telemetry. This regresses observability for non-cancel auth failures: they are still classified as warnings (warnError) but the new predicate short-circuits them first.
Useful? React with 👍 / 👎.
dieguezguille
force-pushed
the
transport
branch
from
de51847 to
fd9ee46
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
from
fd9ee46 to
ebb61b1
Compare
dieguezguille
force-pushed
the
transport
branch
from
ebb61b1 to
20f2565
Compare
dieguezguille
force-pushed
the
transport
branch
from
20f2565 to
381d1b1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 381d1b1abe
ℹ️ 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".
| data: encodeFunctionData(execute), | ||
| chainId: requestedChainId, | ||
| }); | ||
| return { id: concat([hash, numberToHex(requestedChainId, { size: 32 }), TX_MAGIC_ID]) }; |
There was a problem hiding this comment.
When wallet_sendCalls falls back to sendTransaction, this path returns a synthetic TX_MAGIC_ID-encoded id that is not produced by wallet_sendCalls; wallet_getCallsStatus in this client only decodes UO_MAGIC_ID and forwards everything else to getCallsStatus(ownerConfig), so the new waitForCallsStatus(...) calls added across repay/swap/send/rollover can fail or time out whenever this fallback is taken. Fresh evidence: src/utils/e2e.ts includes a dedicated TX_MAGIC_ID decode branch for status polling, but accountClient does not.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit