Viem Migration - Failed onchain TXs are not detected#7421
Conversation
|
@lgahdl is attempting to deploy a commit to the cow Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds detection for transactions never broadcast to chain, a new TransactionNotBroadcastError, grace-period-based cancellation that emits a TX_CANCELLED_NOT_BROADCAST event, UI notification handling for that event, plus related receipt-polling, reducer, hook, and minor utility changes. (≤50 words) ChangesTX Cancellation & Receipt Flow
Sequence DiagramsequenceDiagram
participant Hook as useGetReceipt Hook
participant RPC as RPC/Provider
participant Checker as checkOnChainTransaction
participant Emitter as EventEmitter
participant Updater as OnchainTransactionEventsUpdater
participant UI as UI/Notification
Hook->>RPC: getTransactionReceipt(hash)
RPC-->>Hook: null (no receipt)
Hook->>RPC: getTransaction(hash)
alt RPC: returns transaction
RPC-->>Hook: transaction (in mempool)
Hook-->>Checker: treat as retryable (no receipt yet)
else RPC: TransactionNotFoundError
RPC-->>Hook: TransactionNotFoundError
Hook-->>Checker: throw TransactionNotBroadcastError
end
Checker->>Checker: compute grace = now - tx.addedTime
alt grace elapsed
Checker->>Emitter: emit(TX_CANCELLED_NOT_BROADCAST, payload)
Emitter-->>Updater: event received
Updater->>Updater: play error sound
Updater->>UI: enqueue localized snackbar with tx link
UI-->>UI: show notification
else not yet elapsed
Checker->>Checker: retry on next poll
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
lgahdl
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @lgahdl , sorry, not fixed :(
https://sepolia.etherscan.io/tx/0xd14d2e16469be51c56a57c8ebd2eaffbe742c8976d12235cad328c1c2e9a6d3f
To reproduce:
- connect to mm
- specify an onchain TX
- send the tx to MM
- before signing, pick an 'advanced' option for gas
- change gaslimit to an extremely low value there
- save and sign the TX --> it will fail.
…ap UX; fix(revert): reverting fix from other commit that didn't work as expected;
Hey, now it should work |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/hooks/usePendingTransactionsContext.ts`:
- Around line 37-39: Don't turn RPC nonce-fetch failures into a valid 0 nonce:
remove the .catch(() => 0) on getTransactionCount so failures produce undefined
(or propagate), type transactionsCount as number | undefined (used by
CheckEthereumTransactions), and update any replacement-detection logic (in
CheckEthereumTransactions and downstream code) to explicitly guard with
transactionsCount !== undefined before treating the nonce as a real count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cee38738-e2c8-4060-8af4-7a86b9d129c6
📒 Files selected for processing (5)
apps/cowswap-frontend/src/common/hooks/useGetReceipt.tsapps/cowswap-frontend/src/modules/onchainTransactions/onchainTransactionsEvents.tsapps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/hooks/usePendingTransactionsContext.tsapps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/services/checkOnChainTransaction.tsapps/cowswap-frontend/src/modules/onchainTransactions/updaters/OnchainTransactionEventsUpdater.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/cowswap-frontend/src/modules/onchainTransactions/onchainTransactionsEvents.ts
| // Fallback to 0 on failure so receipt checking can still run even when the nonce fetch fails | ||
| // (e.g. temporary RPC errors). The nonce-based replacement check will simply be skipped. | ||
| const transactionsCount = await getTransactionCount(config, { address: account }).catch(() => 0) |
There was a problem hiding this comment.
Don’t map nonce-fetch failures to 0
Line 39 turns RPC failure into a valid nonce value, which makes the replacement check indistinguishable from a true fresh-account state. That can suppress replacement detection while the RPC is degraded.
Suggested direction
- const transactionsCount = await getTransactionCount(config, { address: account }).catch(() => 0)
+ const transactionsCount = await getTransactionCount(config, { address: account }).catch(() => undefined)Then type transactionsCount as number | undefined in CheckEthereumTransactions and guard downstream replacement logic with transactionsCount !== undefined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/cowswap-frontend/src/modules/onchainTransactions/updaters/FinalizeTxUpdater/hooks/usePendingTransactionsContext.ts`
around lines 37 - 39, Don't turn RPC nonce-fetch failures into a valid 0 nonce:
remove the .catch(() => 0) on getTransactionCount so failures produce undefined
(or propagate), type transactionsCount as number | undefined (used by
CheckEthereumTransactions), and update any replacement-detection logic (in
CheckEthereumTransactions and downstream code) to explicitly guard with
transactionsCount !== undefined before treating the nonce as a real count.
Done |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/common-utils/src/misc.ts`:
- Line 130: The current return uses error.message as string without verifying
its runtime type; update the guard to check typeof error.message === 'string'
before returning it and otherwise fall back to a safe string (e.g.,
String(error) or ''), so callers like isRejectRequestProviderError won't crash
when calling string methods; locate the check around the variable error in
libs/common-utils/src/misc.ts and replace the unconditional cast with a typeof
check and safe fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ced9596e-0ed1-465d-bc67-e05e2679b1f5
📒 Files selected for processing (1)
libs/common-utils/src/misc.ts
| // Prefer viem's shortMessage (concise, human-readable) over the full message | ||
| // which includes verbose request arguments and hex data. | ||
| if ('shortMessage' in error && typeof error.shortMessage === 'string') return error.shortMessage | ||
| if ('message' in error) return error.message as string |
There was a problem hiding this comment.
Guard message type before returning to avoid downstream crashes
At Line 130, error.message as string can return non-string values at runtime. That can break consumers that call string methods (e.g., toLowerCase() in isRejectRequestProviderError). Please add a runtime typeof check.
Suggested fix
- if ('message' in error) return error.message as string
+ if ('message' in error && typeof error.message === 'string') return error.message📝 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.
| if ('message' in error) return error.message as string | |
| if ('message' in error && typeof error.message === 'string') return error.message |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/common-utils/src/misc.ts` at line 130, The current return uses
error.message as string without verifying its runtime type; update the guard to
check typeof error.message === 'string' before returning it and otherwise fall
back to a safe string (e.g., String(error) or ''), so callers like
isRejectRequestProviderError won't crash when calling string methods; locate the
check around the variable error in libs/common-utils/src/misc.ts and replace the
unconditional cast with a typeof check and safe fallback.
3 participants
Summary
Notion Issue: https://www.notion.so/cownation/Failed-onchain-TXs-are-not-detected-34b8da5f04ca808b9bb1e0a30859f213
Fix wrap/unwrap transaction errors not showing in the UI
When a wrap or unwrap transaction fails before being broadcast (e.g. viem simulates the contract call internally and detects a revert due to low gas price), the error was thrown but never caught by the button's click handler, resulting in an unhandled promise rejection with no user feedback.
Additionally, for transactions that were broadcast and mined with
status: reverted, the Redux state was not marking them as failed (errorMessagewas never set), causing the activity panel to show them in an undefined state instead ofFAILED.To Test
Background
wrapUnwrapCallbackre-throws non-rejection errors after logging them. TheWrapUnwrapFlowbutton intradeButtonsMap.tsxwas callingprops.wrapNativeFlow()without handling the returned promise, so errors silently became unhandled rejections with no UI feedback.Two fixes were applied:
tradeButtonsMap.tsx: Added.catch()to the wrap/unwrap button click handler that displays the error viauseAddSnackbar. Uses viem'sshortMessagewhen available for a cleaner error message.enhancedTransactions/reducer.ts: WhenfinalizeTransactionis dispatched with a receipt ofstatus: 'reverted', the transaction'serrorMessageis now set to'Transaction failed', ensuringgetTxActivityStatuscorrectly returnsActivityStatus.FAILEDfor the activity panel.Summary by CodeRabbit
New Features
Bug Fixes