allow cancelling userops #918
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughRefactors the cancellation route to branch by transaction.status (“queued” vs “sent”) instead of isUserOp, removes retries from the send queue for queued items, conditionally builds a unified cancelledTransaction payload (handling isUserOp fields), optionally sends an on-chain cancellation for sent non-userOp transactions, and standardizes the returned hash selection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Route as Cancel Route
participant Queue as SEND_TRANSACTION Queue
participant Chain as Chain/Provider
Client->>Route: POST /transaction/:id/cancel
alt status == "queued"
Route->>Queue: remove all retries for tx
Note right of Route: Build cancelledTransaction<br/>(status="cancelled", timestamps)
alt isUserOp
Note right of Route: Set userOpHash, isUserOp=true
else not isUserOp
Note right of Route: nonce=-1, isUserOp=false,<br/>sentTransactionHashes=[]
end
Route-->>Client: 200 with hash = userOpHash or last sent hash (none)
else status == "sent"
alt isUserOp
Note right of Route: No on-chain cancel<br/>(populate userOp fields)
Route-->>Client: 200 with hash = userOpHash
else not isUserOp
Route->>Chain: send cancellation transaction
Chain-->>Route: cancellation txHash
Note right of Route: Append txHash to sentTransactionHashes
Route-->>Client: 200 with hash = last sentTransactionHashes
end
else other status
Route-->>Client: 4xx error (non-cancellable)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/routes/transaction/cancel.ts (1)
116-131: Preserve previously sent hashes; append the cancellation hash instead of replacingReplacing the array drops historical hashes. Append the new cancellation hash.
cancelledTransaction = { ...transaction, status: "cancelled", cancelledAt: new Date(), - sentTransactionHashes: [transactionHash], + sentTransactionHashes: [ + ...(transaction.sentTransactionHashes ?? []), + transactionHash, + ], };
🧹 Nitpick comments (3)
src/server/routes/transaction/cancel.ts (3)
85-95: Remove off‑by‑one risk and speed up queue cleanupUse <= to cover the last retry index and parallelize removals to reduce handler latency. Also, consider the race where a job is “currently running” and not removed—ensure the worker checks the DB status before sending.
- for ( - let resendCount = 0; - resendCount < config.maxRetriesPerTx; - resendCount++ - ) { - await SendTransactionQueue.remove({ queueId, resendCount }); - } + const removals = Array.from( + { length: config.maxRetriesPerTx + 1 }, + (_v, resendCount) => SendTransactionQueue.remove({ queueId, resendCount }), + ); + await Promise.all(removals);Follow‑up:
- Verify whether resendCount is inclusive of maxRetriesPerTx in your enqueue flow. If it is exclusive, keep
<; if inclusive, prefer<=as suggested.- Confirm the worker aborts sending when it observes a DB status of "cancelled" (to avoid sending a just‑cancelled tx). I can help add a guard if needed.
152-155: Avoid returning a fabricated zero userOpHash; prefer truthy hash resolutionUse a truthy coalesce so queued userOps don’t return a misleading zero hash. This also naturally yields undefined for queued items.
- transactionHash: - "sentTransactionHashes" in cancelledTransaction - ? cancelledTransaction.sentTransactionHashes.at(-1) - : cancelledTransaction.userOpHash, + transactionHash: + cancelledTransaction.sentTransactionHashes?.at(-1) ?? + cancelledTransaction.userOpHash,
172-174: Guard msSinceSend for never‑sent transactionsComputing msSince on a fabricated sentAt skews metrics. Only compute when sentAt exists.
- msSinceSend: msSince(cancelledTransaction.sentAt), + msSinceSend: cancelledTransaction.sentAt + ? msSince(cancelledTransaction.sentAt) + : undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/server/routes/transaction/cancel.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/server/routes/transaction/cancel.ts (2)
src/worker/queues/send-transaction-queue.ts (1)
SendTransactionQueue(11-41)src/shared/utils/block.ts (1)
getBlockNumberish(13-33)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
Comment on lines
+96
to
+115
| cancelledTransaction = { | ||
| ...transaction, | ||
| status: "cancelled", | ||
| cancelledAt: new Date(), | ||
|
|
||
| // Dummy data since the transaction was never sent. | ||
| sentAt: new Date(), | ||
| sentAtBlock: await getBlockNumberish(transaction.chainId), | ||
| // Dummy data since the transaction was never sent. | ||
| sentAt: new Date(), | ||
| sentAtBlock: await getBlockNumberish(transaction.chainId), | ||
|
|
||
| isUserOp: false, | ||
| gas: 0n, | ||
| nonce: -1, | ||
| sentTransactionHashes: [], | ||
| }; | ||
| } else if (transaction.status === "sent") { | ||
| // isUserOp: false, | ||
| gas: 0n, | ||
| ...(transaction.isUserOp | ||
| ? { | ||
| userOpHash: | ||
| "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
| nonce: "cancelled", | ||
| isUserOp: true, | ||
| } | ||
| : { nonce: -1, sentTransactionHashes: [], isUserOp: false }), | ||
| }; |
There was a problem hiding this comment.
Don’t overwrite nonce/gas and avoid dummy “sent” fields for queued cancels
Overwriting nonce with -1/"cancelled", forcing gas to 0n, and fabricating sentAt/sentAtBlock introduces data loss and distorts analytics/webhooks. Preserve original fields; avoid unnecessary RPC calls for queued items; and don’t inject a zero userOpHash.
cancelledTransaction = {
...transaction,
status: "cancelled",
cancelledAt: new Date(),
-
- // Dummy data since the transaction was never sent.
- sentAt: new Date(),
- sentAtBlock: await getBlockNumberish(transaction.chainId),
-
- // isUserOp: false,
- gas: 0n,
- ...(transaction.isUserOp
- ? {
- userOpHash:
- "0x0000000000000000000000000000000000000000000000000000000000000000",
- nonce: "cancelled",
- isUserOp: true,
- }
- : { nonce: -1, sentTransactionHashes: [], isUserOp: false }),
+ // Preserve original nonce/gas; do not fabricate "sent" metadata.
+ ...(transaction.isUserOp
+ ? {
+ // keep existing userOp-specific fields if present; don't fabricate a hash
+ isUserOp: true,
+ }
+ : {
+ isUserOp: false,
+ // keep any existing hashes array if present
+ sentTransactionHashes: transaction.sentTransactionHashes ?? [],
+ }),
};Notes:
- If CancelledTransaction currently requires sentAt/sentAtBlock, consider making them optional or handle “never sent” in downstream analytics instead of fabricating values. I can help adjust the type and usages.
🤖 Prompt for AI Agents
In src/server/routes/transaction/cancel.ts around lines 96-115, do not overwrite
nonce or gas or fabricate sentAt/sentAtBlock/userOpHash for queued cancels:
preserve the original transaction.nonce and transaction.gas, remove the sentAt
and sentAtBlock assignments and the dummy userOpHash/nonce="cancelled"/nonce:-1
values, and set isUserOp appropriately from transaction.isUserOp; also remove
the await getBlockNumberish(...) RPC call for cancels that were never sent. If
the CancelledTransaction type currently requires sentAt/sentAtBlock or a
userOpHash, update the type to make those fields optional or adjust downstream
consumers to handle "never sent" cases instead of injecting fake values.
joaquim-verges
approved these changes
Sep 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR refines the
cancelTransactionfunction in thesrc/server/routes/transaction/cancel.tsfile to improve the handling of transaction cancellations, especially for user operations. It ensures that the cancellation logic is clearer and correctly differentiates between user operations and standard transactions.Detailed summary
transaction.isUserOpbefore handling the "queued" status.cancelledTransactionby merging properties conditionally based onisUserOp.transactionHashfor the response based on whethercancelledTransactioncontainssentTransactionHashesoruserOpHash.Summary by CodeRabbit
New Features
Bug Fixes