refactor: timeouts and logging in wallet capabilities#7821
Open
kernelwhisperer wants to merge 1 commit into
Open
refactor: timeouts and logging in wallet capabilities#7821kernelwhisperer wants to merge 1 commit into
kernelwhisperer wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThe PR replaces ChangesTimeout migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant walletCapabilitiesAtom
participant ViemProvider
participant withTimeout
participant LegacyRPC
walletCapabilitiesAtom->>ViemProvider: getCapabilities()
walletCapabilitiesAtom->>withTimeout: enforce REQUEST_TIMEOUT_MS
withTimeout-->>walletCapabilitiesAtom: capabilities or TimeoutError
walletCapabilitiesAtom->>LegacyRPC: wallet_getCapabilities request
walletCapabilitiesAtom->>withTimeout: enforce REQUEST_TIMEOUT_MS
withTimeout-->>walletCapabilitiesAtom: capabilities or TimeoutError
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Deploying explorer-dev with
|
| Latest commit: |
5a1c3f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dc9b8da7.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://refactor-wallet-capabilities.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
5a1c3f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eef8c655.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://refactor-wallet-capabilities.swap-dev-5u6.pages.dev |
Base automatically changed from
refactor/COW-60-orders-table-joi-redux-sync
to
develop
July 9, 2026 14:14
kernelwhisperer
force-pushed
the
refactor/wallet-capabilities
branch
from
f858fc6 to
67a1a37
Compare
kernelwhisperer
changed the title
kernelwhisperer
force-pushed
the
refactor/wallet-capabilities
branch
from
67a1a37 to
d76cdd3
Compare
kernelwhisperer
commented
Jul 10, 2026
| console.error('Cannot fetch wallet capabilities', getCapabilitiesError) | ||
| throw getCapabilitiesError | ||
| logWallet.error('Cannot fetch wallet capabilities via wagmi', { account, chainId }, wagmiError) | ||
| return null |
Contributor
Author
There was a problem hiding this comment.
This is the only breaking changes, instead of throw getCapabilitiesError we return null. It's what we do in other error cases and what the consumers expect.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/wallet/src/api/state/walletCapabilitiesAtom.ts (1)
60-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTimeout message format inconsistency.
The widget meta-info timeout message uses
${REQUEST_TIMEOUT_MS}ms(→ "5000ms") while the capabilities timeout message at line 171 uses${REQUEST_TIMEOUT_MS / 1000}s(→ "5s"). Both are correct but inconsistent — consider aligning the format for uniform log output.♻️ Suggested alignment
- timeoutMessage: `Widget provider meta info request timed out after ${REQUEST_TIMEOUT_MS}ms`, + timeoutMessage: `Widget provider meta info request timed out after ${REQUEST_TIMEOUT_MS / 1000}s`,🤖 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/wallet/src/api/state/walletCapabilitiesAtom.ts` around lines 60 - 62, Align the timeout message formats in the widget meta-info request and capabilities request. Update the timeout message near providerMetaInfo and the corresponding capabilities timeout in the relevant function to use the same unit and interpolation style, preferably seconds via REQUEST_TIMEOUT_MS / 1000, while preserving the existing timeout behavior.
🤖 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.
Nitpick comments:
In `@libs/wallet/src/api/state/walletCapabilitiesAtom.ts`:
- Around line 60-62: Align the timeout message formats in the widget meta-info
request and capabilities request. Update the timeout message near
providerMetaInfo and the corresponding capabilities timeout in the relevant
function to use the same unit and interpolation style, preferably seconds via
REQUEST_TIMEOUT_MS / 1000, while preserving the existing timeout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d57a21a-ce3d-4772-989a-499bcc67ea36
📒 Files selected for processing (8)
apps/explorer/src/api/operator/operatorApi.tslibs/common-utils/src/index.tslibs/common-utils/src/misc.test.tslibs/common-utils/src/misc.tslibs/common-utils/src/promiseWithTimeout.test.tslibs/common-utils/src/promiseWithTimeout.tslibs/wallet/src/api/state/walletCapabilitiesAtom.test.tslibs/wallet/src/api/state/walletCapabilitiesAtom.ts
💤 Files with no reviewable changes (3)
- libs/common-utils/src/promiseWithTimeout.test.ts
- libs/common-utils/src/promiseWithTimeout.ts
- libs/common-utils/src/index.ts
Contributor
There was a problem hiding this comment.
✅ AI Review follow-up: Safe-via-WalletConnect QA verified
Resolution: addressed. The requested browser QA evidence now covers the distinct Safe capability paths on PR head d76cdd3:
- Safe App: detected the Safe and atomic batching support, then exposed TWAP onboarding. As expected, this path bypassed the changed capability fallback.
- Safe via WalletConnect, ordinary flow: reached wallet-connected UI, invoked the direct
wallet_getCapabilitiesfallback, resolved batching as supported, and exposed TWAP onboarding. - Safe via WalletConnect, timeout A/B: with only viem's capability request held pending, the PR timed out, invoked the real Safe provider's direct RPC fallback, resolved batching as supported, and opened the normal TWAP form. The pinned
developcontrol did not invoke the fallback and showedUse Safe web app/Unsupported wallet.
Conclusion
The QA evidence gap is closed and no code change is requested from this finding. The run confirms the original technical observation: allowing a viem timeout to enter the direct RPC fallback is a behavior change, despite the PR description calling the refactor logic-neutral, and the changed behavior is desirable for Safe via WalletConnect.
The run did not submit a batch transaction, signature, or order. That remains outside this capability-detection verification.
Original review rationale and related context
- Location:
libs/wallet/src/api/state/walletCapabilitiesAtom.ts:176 - Safe App returns early through
isSafeAppAtom, so testing only “a Safe wallet” could pass without exercising the changed fallback. - The common-utils and wallet test suites passed, including the new timeout and fallback cases.
- The PR's Cypress failures matched the same four missing-trade-button failures on the tested
developcontrol, so they were not attributable to this diff. - The author's existing thread covered the separate null-vs-throw behavior change.
- CodeRabbit covered the timeout-message unit inconsistency; it is not repeated here.
Generated using the pr-review and pr-qa skills from the CoW Protocol skills repo.
Contributor
✅ Browser QA passed: Safe App and Safe-via-WalletConnect timeout fallbackOutcome
Run details
Artifacts
How to retest
Generated using the |
fairlighteth
approved these changes
Jul 10, 2026
kernelwhisperer
force-pushed
the
refactor/wallet-capabilities
branch
from
86d6ba4 to
5a1c3f9
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
To Test
This PR doesn't really change any logic, just a refactor.
Background
In the next PR (#7844) I will implement https://linear.app/cowswap/issue/FE-265/batch-support-for-eoas-and-eip7702-delegated-eoas
Summary by CodeRabbit
Summary by CodeRabbit
nullwhen providers fail or time out.withTimeoutAPI andTimeoutError.PromiseWithTimeoututility export and updated related tests.