feat: add balancec-watcher updater [pr 2 - connection logic] - #7640
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an SSE-driven balances watcher mode to the ChangesSSE Balances Watcher Mode
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
note over CommonPriorityBalancesAndAllowancesUpdater: Feature flag active
CommonPriorityBalancesAndAllowancesUpdater->>BalancesWatcherUpdater: render(account, chainId)
end
BalancesWatcherUpdater->>useEnabledTokensListsUrls: get enabled list URLs
BalancesWatcherUpdater->>useCustomTokensForChain: get custom token addresses
BalancesWatcherUpdater->>useBalancesWatcherSession: start(account, chainId, listUrls, customTokens)
useBalancesWatcherSession->>createBalancesWatcherSession: POST session
createBalancesWatcherSession-->>useBalancesWatcherSession: sessionId
useBalancesWatcherSession->>subscribeToBalancesEvents: open SSE stream
subscribeToBalancesEvents-->>useBalancesWatcherSession: balance diff event
useBalancesWatcherSession->>balancesAtom: merge update (BigInt, normalized addresses)
BalancesWatcherUpdater->>NativeTokenBalanceUpdater: render(account, chainId)
NativeTokenBalanceUpdater->>useNativeTokenBalance: fetch native balance
NativeTokenBalanceUpdater->>balancesAtom: write native token balance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 swap-dev with
|
| Latest commit: |
4dda8df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7524de8d.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://feat-balance-watcher-updater.swap-dev-5u6.pages.dev |
…-watcher-updater-2 # Conflicts: # libs/balances-and-allowances/src/balancesWatcher/subscribeToBalancesEvents.test.ts
Deploying explorer-dev with
|
| Latest commit: |
4dda8df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f591fe1e.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://feat-balance-watcher-updater.explorer-dev-dxz.pages.dev |
elena-zh
left a comment
There was a problem hiding this comment.
Thank you, approved with issues to be addressed separately!
fixed, thank you |
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 4m): empty watcher token set can keep balances loading forever
Finding: Mark watcher mode loaded when there are no ERC20 tokens to subscribe to
- Location:
libs/balances-and-allowances/src/hooks/useBalancesWatcherSession.ts:43 - When
isBwEnabledis on,BalancesWatcherUpdaterreplaces the RPC updater for EVM chains. - If a connected user has no enabled token-list URLs and no custom tokens,
useBalancesWatcherSessionreturns before creating a session and never updatesbalancesAtom. useTradeFormValidationContextstill treats!hasFirstLoadas loading, so the swap UI can stay inBalancesLoadingforever even thoughNativeTokenBalanceUpdatermay separately write the native balance.
Suggested fix
- In the empty
tokensListsUrls+customTokenscase, transitionbalancesAtomto a completed empty-load state for the currentchainId(hasFirstLoad: true,isLoading: false,error: null) instead of returning without touching state. - Add a focused test for a connected EVM account with both arrays empty, asserting the watcher does not create a session and the first-load gate is closed.
Review scope and related context
Already covered or rechecked, so not repeated as new findings:
- The previous AI finding about pre-snapshot watcher failures is fixed on the current head: terminal SSE errors and
createSessionrejection now callapplyTerminalError, which setshasFirstLoad: true. - The
toLowerCase()test normalization issue is resolved withgetAddressKey. - The updater-vs-atom architecture discussion is already active in Danziger's review threads.
- CodeRabbit already raised the non-blocking point that legacy hooks still run before the watcher-mode return.
Local targeted tests were not runnable in this checkout because dependencies are not installed (node_modules / Nx / Jest missing). GitHub Test/Lint/Typecheck/Cypress were still pending when checked.
🤖 Prompt for AI agents
Verify this finding against the current PR head. Fix only if still valid, keep the change minimal, and validate with a focused test.
Context:
- `CommonPriorityBalancesAndAllowancesUpdater` renders `BalancesWatcherUpdater` for EVM chains when `isBwEnabled` is true.
- `useBalancesWatcherSession` returns early when `tokensListsUrls.length === 0 && customTokens.length === 0`.
- That early return does not set `hasFirstLoad: true`.
- `useTradeFormValidationContext` treats `!hasFirstLoad` as balances loading.
- Expected behavior: connected EVM watcher mode with no ERC20 tokens to subscribe to should complete an empty balances load instead of leaving the swap form stuck in loading.
Generated using the pr-review skill from the CoW Protocol skills repo.
fairlighteth
left a comment
There was a problem hiding this comment.
✅ Security Review (Codex GPT-5, worked 1m): no verified security findings
Review completed. I found no verified security findings worth posting.
Review scope and related context
- Checked: balances-watcher session creation, SSE URL construction, malformed
balance_updatehandling, terminal/non-terminal error handling, stale session cleanup, token-list/custom-token data sent to the watcher, and workflow env wiring forREACT_APP_BALANCES_WATCHER_BASE_URL. - Source: code inspection only, plus current GitHub PR checks.
- Lifecycle: no
verified-vulnerabilityfindings. - Existing comments: the remaining issue I found earlier is availability/UX (
hasFirstLoadstays false with an empty tracked token set), not a verified security vulnerability. - CI context: lint, tests, typecheck, agent harness, Socket checks, and deploy previews pass on current head; Cypress was still pending when checked.
- Confidence limits: I did not inspect the balances-watcher backend, so backend-side handling of user-enabled token-list URLs is outside this frontend PR review.
Generated using the security-review skill from the CoW Protocol skills repo.
|
@limitofzero one more new finding #7640 (review) please review if applicable or not. |
thank you, fixed |
…ol/cowswap into feat/balance-watcher-updater-2
fairlighteth
left a comment
There was a problem hiding this comment.
✅ AI Review (Codex GPT-5, worked 1m): follow-up addressed
Rechecked
- Author response: @limitofzero said the empty watcher token-set finding was fixed.
- Code path:
libs/balances-and-allowances/src/hooks/useBalancesWatcherSession.ts:43. - Test coverage:
libs/balances-and-allowances/src/hooks/useBalancesWatcherSession.test.tsx:110.
Result: Fixed. The empty token-list/custom-token case now skips session creation but still closes the first-load gate with hasFirstLoad: true, isLoading: false, error: null, and the current chainId.
🤖 Verification notes for AI agents
Verify the prior finding against current code only. Confirm whether the author's claimed fix addresses the specific failure mode, and avoid reopening broader or unrelated issues.
Prior finding:
- Connected EVM watcher mode with no enabled token-list URLs and no custom tokens returned before updating balancesAtom, leaving useTradeFormValidationContext stuck in BalancesLoading.
Current verification:
- useBalancesWatcherSession now calls applyEmptyLoad(state, chainId) before returning from the empty arrays branch.
- The focused test asserts no session is created and hasFirstLoad is true, isLoading is false, error is null, and chainId is set.
Generated using the pr-review skill from the CoW Protocol skills repo.

Summary
Wires the cowswap-frontend to the new balances-watcher SSE service, behind the LaunchDarkly flag isBwEnabled.
When the flag is on, balances are pushed in real time over an EventSource and the existing multicall pipeline (priority tokens + full token-list polling) is bypassed. When off, nothing changes — the multicall path stays as is.
Note on the native token (ETH / xDAI / MATIC / BNB): the watcher service does not emit native balances, so we keep polling them via a single eth_getBalance RPC call (wagmi useBalance), refetched every 11 seconds.
This means the native balance can lag up to 11s behind the on-chain state — that's an explicit trade-off because I reused current implementation, but if it's crucial - I can rework it.
Out of scope (planned for follow-up):
To Test
Summary by CodeRabbit
Release Notes