fix: batch breaks on safe wallets after network change #6763
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe hook Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/wallet/src/api/hooks/useWalletCapabilities.ts (1)
44-48: Clarify the SWR key/fetcher mismatch or remove chainId from the cache key.The SWR cache key includes
chainId(line 46), causing refetches when the chain changes, but the fetcher ignores it entirely (line 48:_chainId). The code always returnsresult[Object.keys(result)[0]], taking the first available capability regardless of the current chain.This creates a semantic gap: if
wallet_getCapabilitiesreturns capabilities for multiple chains (per EIP-5792), the returned capabilities may not match the currentchainId. InuseSendBatchTransactions, these capabilities are then used with the currentchainIdforwallet_sendCalls, which could cause incorrect behavior or failures.Either:
- Remove
chainIdfrom the SWR key if it's not needed for the lookup- Use
chainIdproperly to select the correct entry from the result:result[chainIdHex]wherechainIdHex = '0x' + (+chainId).toString(16)- Add a comment explaining why this mismatch is necessary (e.g., Safe wallet workaround) and document the assumptions about which chain's capabilities are returned
🧹 Nitpick comments (1)
libs/wallet/src/api/hooks/useWalletCapabilities.ts (1)
54-65: Consider a more explicit fallback when provider returns multiple chains.The
wallet_getCapabilitiesRPC call is made with only the account parameter (no chainId), so the provider's response can contain capabilities for multiple chains. The current approach of selecting the first key works as a practical workaround for Safe wallet, but it has minor robustness concerns:
- Unclear selection: If the provider returns multiple chains, which chain's capabilities are actually used is undefined.
- Object.keys() ordering: While deterministic in modern JavaScript (insertion order), the order is not guaranteed to be the intended chain.
The
_chainIdparameter is available and could be used to prefer the current chain's capabilities if present. A simple improvement:- resolve(result[Object.keys(result)[0]]) + const currentChainHex = `0x${_chainId.toString(16)}` + resolve(result[currentChainHex] ?? result[Object.keys(result)[0]])This prefers the current chain when available while maintaining the Safe wallet fallback.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/wallet/src/api/hooks/useWalletCapabilities.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5992
File: libs/wallet/src/web3-react/utils/switchChain.ts:36-38
Timestamp: 2025-08-05T14:27:05.023Z
Learning: In libs/wallet/src/web3-react/utils/switchChain.ts, the team prefers using Record<SupportedChainId, string | null> over Partial<Record<SupportedChainId, string>> for WALLET_RPC_SUGGESTION to enforce that all supported chain IDs have explicit values set, even if some might be null. This ensures compile-time completeness checking.
📚 Learning: 2025-08-05T14:27:05.023Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5992
File: libs/wallet/src/web3-react/utils/switchChain.ts:36-38
Timestamp: 2025-08-05T14:27:05.023Z
Learning: In libs/wallet/src/web3-react/utils/switchChain.ts, the team prefers using Record<SupportedChainId, string | null> over Partial<Record<SupportedChainId, string>> for WALLET_RPC_SUGGESTION to enforce that all supported chain IDs have explicit values set, even if some might be null. This ensures compile-time completeness checking.
Applied to files:
libs/wallet/src/api/hooks/useWalletCapabilities.ts
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.
Applied to files:
libs/wallet/src/api/hooks/useWalletCapabilities.ts
📚 Learning: 2025-08-12T05:57:08.021Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6138
File: libs/hook-dapp-lib/src/hookDappsRegistry.ts:1-1
Timestamp: 2025-08-12T05:57:08.021Z
Learning: The matchHooksToDapps function in libs/hook-dapp-lib/src/utils.ts provides backward compatibility for permit hooks through function selector detection (EIP_2612_PERMIT_SELECTOR and DAI_PERMIT_SELECTOR) rather than dappId matching, making it robust against dappId changes.
Applied to files:
libs/wallet/src/api/hooks/useWalletCapabilities.ts
⏰ 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). (3)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
kernelwhisperer
force-pushed
the
fix/batch-tx-missing-from-safe-wallets
branch
from
3f2f498 to
0725ac4
Compare
Summary
Fixes #6758
But I think the issue should be fixed on the Safe/WalletConnect side.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.