Conversation
🦋 Changeset detectedLatest commit: e79dd5b 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 71.96% 71.86% -0.11%
==========================================
Files 229 229
Lines 8389 8512 +123
Branches 2713 2762 +49
==========================================
+ Hits 6037 6117 +80
- Misses 2121 2156 +35
- Partials 231 239 +8
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.
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
eb6d3a6 to
227ff76
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dieguezguille
force-pushed
the
transport
branch
from
227ff76 to
4bcfa86
Compare
dieguezguille
force-pushed
the
transport
branch
from
4bcfa86 to
c19bd5d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c19bd5d9db
ℹ️ 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".
| tokenSelectionType === "to" | ||
| ? selected.address === fromToken.token.address | ||
| ? parseUnits(formatUnits(old.fromAmount, fromToken.token.decimals), toToken.token.decimals) | ||
| : old.fromAmount |
There was a problem hiding this comment.
This branch now reuses old.fromAmount when a user selects the current source token from the “to” picker, but after this refactor fromAmount is no longer kept in sync with route results when editing the “to” side. In that flow, handleAmountChange leaves old.fromAmount stale (often 0n), so swapping token sides can reset or miscompute the amount and then drive incorrect quote/simulation inputs. This should use the current derived fromAmount (or persist it before updating tokens) instead of relying on old.fromAmount.
Useful? React with 👍 / 👎.
dieguezguille
force-pushed
the
transport
branch
from
c19bd5d to
e79dd5b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e79dd5b5f1
ℹ️ 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".
| burntOptions: { haptic: "error", preset: "error" }, | ||
| }); | ||
| reportError(error); | ||
| if (!reportError(error).authKnown) |
There was a problem hiding this comment.
handleError now calls reportError(error) unconditionally before checking authKnown, so wallet/passkey cancellations during bridge or transfer are still sent to Sentry as warnings. In this flow, user-initiated rejects are expected and can happen frequently (especially when retrying quotes), so this change introduces telemetry noise compared to the previous early-return cancellation guards. Classify first and skip reportError for auth-cancelled cases.
Useful? React with 👍 / 👎.
| }, | ||
| }, | ||
| }); | ||
| setEnableSimulations(false); | ||
| const { status } = await waitForCallsStatus(exa, { id }); | ||
| if (status === "failure") throw new Error("failed to repay with external asset"); | ||
| }, | ||
| onMutate() { | ||
| setEnableSimulations(false); | ||
| if (!externalAsset) return; | ||
| if (!route?.fromAmount) return; | ||
| setDisplayValues({ | ||
| amount: Number(route.fromAmount) / 10 ** externalAsset.decimals, | ||
| usdAmount: (Number(externalAsset.priceUSD) * Number(route.fromAmount)) / 10 ** externalAsset.decimals, | ||
| }); | ||
| }, | ||
| onSettled() { | ||
| setEnableSimulations(true); | ||
| }, | ||
| onError(error) { | ||
| reportError(error); | ||
| if (reportError(error).authKnown) resetExternalRepay(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🚩 Repay external asset mutation missing onSuccess query invalidation
The repayWithExternalAsset mutation in Repay.tsx:443-508 does not invalidate assetQueryKey on success, while the non-external repay mutation does (Repay.tsx:432-434). This means after a successful external repay, the asset data won't refresh until the next automatic refetch. However, this is NOT a regression — the old code's repayWithExternalAsset also lacked onSuccess query invalidation. The non-external repay previously used useWriteContract which had the invalidation. Consider adding onSuccess to repayWithExternalAsset for consistency.
(Refers to lines 443-508)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary by CodeRabbit