feat: handle unsupported chain id bff error + update supported chain list #6595
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds explicit unsupported-chain handling for BFF balance fetching: new UnsupportedChainError type, atom-based tracking of unsupported chain IDs, a runtime hook to check BFF support, SWR retry suppression for unsupported-chain errors, and updated fetch hook/updater and tests to use the new flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as BalancesAndAllowancesUpdater
participant Hook as usePersistBalancesFromBff
participant SWR as SWR (bff-balances config)
participant BFF as BFF API
participant State as bffUnsupportedChainsAtom
Updater->>Hook: mount/use with chainId
Hook->>Hook: call useIsBffSupportedNetwork(chainId)
alt chain marked unsupported (static or atom)
Hook->>SWR: supply null key (skip request)
else supported
Hook->>SWR: start fetch (key -> getBffBalances)
SWR->>BFF: HTTP request
BFF-->>SWR: response (OK or error)
SWR-->>Hook: deliver response/error
alt UnsupportedChainError detected
Hook->>State: useAddUnsupportedChainId(chainId)
SWR->>SWR: onErrorRetry detects unsupported message -> early return (no retry)
else other error
SWR->>SWR: onErrorRetry schedules exponential backoff retry
else success
Hook->>Hook: parse and persist balances
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit 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
🧹 Nitpick comments (1)
libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts (1)
7-8: Consider updating or removing the TODO comment.The TODO references checking before Plasma launch (2025/10/20), but this PR is implementing dynamic chain support tracking. Consider whether this TODO is still relevant or should be updated to reflect the new dynamic tracking approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/balances-and-allowances/src/constants/bff-balances-swr-config.ts(1 hunks)libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts(4 hunks)libs/balances-and-allowances/src/state/isBffFailedAtom.ts(2 hunks)libs/balances-and-allowances/src/updaters/BalancesAndAllowancesUpdater.tsx(3 hunks)libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/balances-and-allowances/src/state/isBffFailedAtom.tslibs/balances-and-allowances/src/utils/isBffSupportedNetwork.tslibs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts
📚 Learning: 2025-08-08T13:55:17.528Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/state/tokens/allTokensAtom.ts:78-78
Timestamp: 2025-08-08T13:55:17.528Z
Learning: In libs/tokens/src/state/tokens/allTokensAtom.ts (TypeScript/Jotai), the team prefers to wait for token lists to initialize (listsStatesListAtom non-empty) before returning tokens. No fallback to favorites/user-added/native tokens should be used when listsStatesList is empty.
Applied to files:
libs/balances-and-allowances/src/state/isBffFailedAtom.tslibs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
libs/balances-and-allowances/src/state/isBffFailedAtom.tslibs/balances-and-allowances/src/utils/isBffSupportedNetwork.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/balances-and-allowances/src/utils/isBffSupportedNetwork.tslibs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.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/balances-and-allowances/src/updaters/BalancesAndAllowancesUpdater.tsxlibs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts
🧬 Code graph analysis (4)
libs/balances-and-allowances/src/state/isBffFailedAtom.ts (1)
libs/widget-lib/src/types.ts (1)
SupportedChainId(4-4)
libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts (2)
libs/widget-lib/src/types.ts (1)
SupportedChainId(4-4)libs/balances-and-allowances/src/state/isBffFailedAtom.ts (1)
bffUnsupportedChainsAtom(16-16)
libs/balances-and-allowances/src/updaters/BalancesAndAllowancesUpdater.tsx (1)
libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts (1)
useIsBffSupportedNetwork(14-17)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts (2)
libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts (1)
useIsBffSupportedNetwork(14-17)libs/balances-and-allowances/src/state/isBffFailedAtom.ts (2)
useSetIsBffFailed(12-14)useAddUnsupportedChainId(18-28)
⏰ 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: Setup
- GitHub Check: Cypress
🔇 Additional comments (8)
libs/balances-and-allowances/src/updaters/BalancesAndAllowancesUpdater.tsx (1)
19-19: LGTM! Clean integration of dynamic BFF support check.The changes correctly integrate the new
useIsBffSupportedNetworkhook to add a runtime check before rendering the BFF updater. This ensures that the BFF updater only runs when both the feature is enabled and the network is supported (both statically and dynamically).Also applies to: 44-44, 76-83
libs/balances-and-allowances/src/state/isBffFailedAtom.ts (1)
4-4: LGTM! Correct immutable Set updates.The new atom and hook follow proper Jotai patterns. The
useAddUnsupportedChainIdhook correctly performs immutable updates by creating a new Set only when adding a chain ID that doesn't already exist, which is an efficient optimization.Also applies to: 16-28
libs/balances-and-allowances/src/constants/bff-balances-swr-config.ts (1)
49-58: LGTM! Proper error-based retry control.The addition of early-return logic for "unsupported chain" errors is well-implemented:
- Defensively checks
error instanceof Errorbefore accessing.message- Uses case-insensitive matching with
.toLowerCase()- Preserves existing exponential backoff for other error types
This prevents unnecessary retries when the chain is fundamentally unsupported by the BFF.
libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts (1)
1-2: LGTM! Clean separation of static and dynamic BFF support checks.The new
useIsBffSupportedNetworkhook correctly combines:
- Static allowlist checking via
isBffSupportedNetwork(chainId)(now only PLASMA)- Dynamic runtime exclusions via the
bffUnsupportedChainsAtomThis allows the system to discover and remember unsupported chains at runtime while maintaining a static baseline.
Also applies to: 5-6, 14-17
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts (4)
29-56: Well-structured error parsing helpers.The error handling utilities are cleanly separated and work together effectively:
isUnsupportedChainErrorprovides centralized detection logicparseErrorResponsesafely extracts error messages with type guardsparseBffResponsegracefully handles JSON parsing failureshandleBffError(typed asnever) ensures proper error propagation
64-64: LGTM! Correct hook integration.Both hooks are properly called at the top level:
useIsBffSupportedNetworkreplaces the static check with dynamic trackinguseAddUnsupportedChainIdprovides the callback for recording unsupported chainsAlso applies to: 67-67
92-101: LGTM! Proper error tracking with complete dependencies.The effect correctly:
- Detects unsupported chain errors using the helper function
- Records the chain ID in the atom for future checks
- Updates the general failure flag
- Includes all necessary dependencies (
error,setIsBffFailed,addUnsupportedChainId,targetChainId)The dependency array is complete and the effect will properly track unsupported chains.
144-172: Robust error handling with proper type narrowing.The
getBffBalancesupdates improve error handling significantly:
- Line 155:
parseBffResponsegracefully handles JSON parsing failures- Line 158:
handleBffErrorcentralizes error logic and ensures consistent error messages- Line 161: The
'balances' in datatype guard properly narrows the union type before accessingdata.balances- Lines 167-169: Specific re-throw of unsupported chain errors preserves error semantics for the SWR retry logic
The error flow is well-coordinated with the SWR config's retry behavior.
limitofzero
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx (3)
187-211: Tighten expectations on SWR calls for unsupported chains (optional)The test correctly verifies that the SWR key is
nullfor an unsupported chain. To guard against regressions where the hook might accidentally call SWR multiple times (e.g. once with a real key and once withnull), you could also assert the call count:- expect(mockUseSWR).toHaveBeenCalledWith( + expect(mockUseSWR).toHaveBeenCalledWith( null, expect.any(Function), BFF_BALANCES_SWR_CONFIG, ) + expect(mockUseSWR).toHaveBeenCalledTimes(1)Not mandatory, but it would make the intent and constraints of this behavior clearer.
213-239: Consider aligning the hard-coded error message with the actual BFF error stringThe test logic and use of
waitForto observebffUnsupportedChainsAtomupdates look good. The only concern is the hard-codednew Error('Unsupported chain'). The PR description mentions an"Unsupported network Id"error from BFF, while this andBFF_BALANCES_SWR_CONFIG.onErrorRetrylook for"unsupported chain".To avoid drift between tests, frontend error handling, and the backend contract, consider:
- Using a shared constant (e.g. exported from the hook or SWR config) for the substring you match on, and referencing it here, or
- Updating the message in the test to exactly match the real BFF error text used in production.
This will keep the tests aligned if the backend message changes or was slightly different to begin with.
241-285: Wrapper duplication for unsupported-chain pre-hydration is fine but could be DRYed upThe custom
wrapperWithUnsupportedChaincorrectly pre-hydratesbffUnsupportedChainsAtomwithMAINNETand verifies that SWR is called with anullkey, which matches the intended behavior.If you want to reduce duplication with the main
wrapper, you could extract a small helper that takes an optionalunsupportedChains: Set<SupportedChainId>and returns a wrapper, e.g.:- const wrapper = ({ children }: { children: ReactNode }): ReactNode => { + const createWrapper = + (unsupportedChains: Set<SupportedChainId> = new Set()) => + ({ children }: { children: ReactNode }): ReactNode => { const HydrateAtoms = ({ children }: { children: ReactNode }): ReactNode => { useHydrateAtoms([ [balancesAtom, { ... } as BalancesState], [balancesUpdateAtom, mockBalancesUpdate], - [bffUnsupportedChainsAtom, new Set<SupportedChainId>()], + [bffUnsupportedChainsAtom, unsupportedChains], ]) return <>{children}</> } return ( <Provider> <HydrateAtoms>{children}</HydrateAtoms> </Provider> ) } + + const wrapper = createWrapper()Then this test can use
wrapper: createWrapper(new Set([SupportedChainId.MAINNET])). Purely a readability/maintenance improvement; current code is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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-09-25T08:49:32.256Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:62-67
Timestamp: 2025-09-25T08:49:32.256Z
Learning: In the cowswap-frontend codebase, when testing hooks that use multiple bridge providers, both providers are always properly mocked as complete jest.Mocked<BridgeProvider<BridgeQuoteResult>> objects with all required methods stubbed, ensuring no undefined returns that could break the hook logic.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 Learning: 2025-09-25T08:48:53.495Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:58-60
Timestamp: 2025-09-25T08:48:53.495Z
Learning: In the cowswap-frontend codebase, when writing SWR tests, the team prefers maximum test isolation by using `provider: () => new Map()` in SWRConfig wrappers, even if it recreates cache on every render, to ensure tests don't share any state.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 Learning: 2025-07-28T16:26:08.051Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6034
File: apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts:44-47
Timestamp: 2025-07-28T16:26:08.051Z
Learning: In the cowswap codebase, using trivial placeholder tests like `it('should be true', () => { expect(true).to.be.true })` in e2e test files is an intentional pattern when disabling broken tests to keep CI green while maintaining build efficiency.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 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/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
🧬 Code graph analysis (1)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx (5)
libs/balances-and-allowances/src/state/isBffFailedAtom.ts (1)
bffUnsupportedChainsAtom(16-16)libs/widget-lib/src/types.ts (1)
SupportedChainId(4-4)libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts (2)
PersistBalancesFromBffParams(21-27)usePersistBalancesFromBff(58-142)libs/balances-and-allowances/src/constants/bff-balances-swr-config.ts (1)
BFF_BALANCES_SWR_CONFIG(27-59)libs/balances-and-allowances/src/state/balancesAtom.ts (2)
balancesAtom(33-33)balancesUpdateAtom(35-37)
⏰ 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: Cypress
- GitHub Check: Setup
🔇 Additional comments (1)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx (1)
1-1: bffUnsupportedChainsAtom wiring in the test wrapper looks solidImporting
useAtomValue/bffUnsupportedChainsAtomand hydrating the atom to an emptySetin the sharedwrappergives the new tests a clean, predictable starting state without affecting existing cases. No changes needed here.Also applies to: 17-17, 53-76
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/trade/containers/TradeBasicConfirmDetails/useTradeBasicConfirmDetailsData.ts (1)
20-22: AlignisInvertedStatetype withuseState’s readonly tupleReact 18’s
useStatereturns areadonlytuple, while the result interface currently declares a mutable tuple type. To avoid potential TS incompatibilities and better reflect the actual type, consider aligning the interface withuseState’s return type:interface UseTradeBasicConfirmDetailsDataResult { - isInvertedState: [boolean, Dispatch<SetStateAction<boolean>>] + isInvertedState: ReturnType<typeof useState<boolean>> amountAfterFees: CurrencyAmount<Currency> amountAfterSlippage: CurrencyAmount<Currency>This keeps the contract accurate without changing runtime behavior.
Also applies to: 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cowswap-frontend/src/modules/trade/containers/TradeBasicConfirmDetails/useTradeBasicConfirmDetailsData.ts(1 hunks)libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx(4 hunks)libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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/balances-and-allowances/src/hooks/usePersistBalancesFromBff.tslibs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 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/balances-and-allowances/src/hooks/usePersistBalancesFromBff.tsapps/cowswap-frontend/src/modules/trade/containers/TradeBasicConfirmDetails/useTradeBasicConfirmDetailsData.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/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/trade/containers/TradeBasicConfirmDetails/useTradeBasicConfirmDetailsData.ts
📚 Learning: 2025-06-23T07:03:50.760Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5859
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts:76-82
Timestamp: 2025-06-23T07:03:50.760Z
Learning: In the useTradeQuotePolling hook, there are two useLayoutEffect hooks that work together: one resets the counter to 0 when the confirmation modal closes, and another automatically triggers pollQuote(false, true) whenever the counter reaches 0. This creates an intentional chain reaction for immediate quote updates.
Applied to files:
apps/cowswap-frontend/src/modules/trade/containers/TradeBasicConfirmDetails/useTradeBasicConfirmDetailsData.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 Learning: 2025-09-25T08:49:32.256Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:62-67
Timestamp: 2025-09-25T08:49:32.256Z
Learning: In the cowswap-frontend codebase, when testing hooks that use multiple bridge providers, both providers are always properly mocked as complete jest.Mocked<BridgeProvider<BridgeQuoteResult>> objects with all required methods stubbed, ensuring no undefined returns that could break the hook logic.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 Learning: 2025-09-25T08:48:53.495Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:58-60
Timestamp: 2025-09-25T08:48:53.495Z
Learning: In the cowswap-frontend codebase, when writing SWR tests, the team prefers maximum test isolation by using `provider: () => new Map()` in SWRConfig wrappers, even if it recreates cache on every render, to ensure tests don't share any state.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
📚 Learning: 2025-07-28T16:26:08.051Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6034
File: apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts:44-47
Timestamp: 2025-07-28T16:26:08.051Z
Learning: In the cowswap codebase, using trivial placeholder tests like `it('should be true', () => { expect(true).to.be.true })` in e2e test files is an intentional pattern when disabling broken tests to keep CI green while maintaining build efficiency.
Applied to files:
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx
🔇 Additional comments (14)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts (8)
13-15: LGTM!Clean imports for the new unsupported chain handling utilities. The modular approach with separate hooks and error types improves maintainability.
17-20: LGTM!The
BalanceResponsetype extension with optionalmessagefield properly supports the new error response parsing.
30-47: LGTM!Well-structured helper functions:
isUnsupportedChainMessageuses case-insensitive matching for robustnessparseErrorResponsesafely extracts message with proper null checksparseBffResponsegracefully handles JSON parse failuresThis addresses the past review comment about extracting
'Unsupported chain'into a dedicated check.
49-57: LGTM!The
handleBffErrorfunction cleanly centralizes error handling and throwsUnsupportedChainErrorwhen appropriate, addressing the past review feedback about using a custom error class.
65-68: LGTM!Good integration of the new
useIsBffSupportedNetworkhook anduseAddUnsupportedChainIdcallback for tracking unsupported chains at runtime.
93-101: LGTM!The effect correctly handles unsupported chain errors by:
- Checking if error is
UnsupportedChainErrorvia type guard- Adding the chain to the unsupported list when detected
- Setting the BFF failed state
The
useCallbackwrapping ofaddUnsupportedChainIdwas addressed per past review feedback in the atom hook implementation.
160-164: LGTM!Proper null safety check for the
balancesfield before returning. The logic correctly returnsnullwhen balances are missing rather than throwing, allowing the caller to handle this case gracefully.
153-164: Network errors from fetch propagate uncaught but are properly handled by SWR retry logic.The fetch call has no try-catch wrapper, so network errors (e.g.,
TypeError: Failed to fetch) propagate directly to SWR'sonErrorRetryhandler, which retries with exponential backoff (3 retries × 30s). This is intentional and correct—theonErrorRetryhandler specifically checks for "Unsupported chain" errors to prevent retrying them, but network errors have different message text and will retry normally, which is the expected behavior.libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.test.tsx (5)
1-18: LGTM!Clean imports for the new testing requirements. The addition of
ProvideranduseAtomValuefrom jotai supports the new atom-based unsupported chain tracking tests.
54-77: LGTM!Good test setup with proper hydration of all relevant atoms including the new
bffUnsupportedChainsAtom. The use ofProviderwithuseHydrateAtomspattern ensures maximum test isolation, which aligns with team preferences.
188-212: LGTM!Good test case verifying that unsupported networks (SEPOLIA) result in a
nullSWR key, preventing unnecessary network requests.
214-240: LGTM!Well-structured test that:
- Creates an
UnsupportedChainErrorinstance- Mocks SWR to return this error
- Uses a helper hook to access both the hook under test and the atom value
- Verifies the chain is added to the unsupported set via
waitForThe 3000ms timeout is reasonable for async effect propagation.
242-286: LGTM!Comprehensive test that pre-populates the unsupported chains atom with
SupportedChainId.MAINNETand verifies that subsequent requests for that chain result in anullSWR key. This validates the "circuit breaker" behavior for known unsupported chains.apps/cowswap-frontend/src/modules/trade/containers/TradeBasicConfirmDetails/useTradeBasicConfirmDetailsData.ts (1)
31-53: Hook implementation looks solidThe hook cleanly derives post-fee/slippage amounts, conditionally computes USD values without breaking hook rules, memoizes
limitPricewith appropriate dependencies, and passes through the network cost suffix props. Aside from the minorisInvertedStatetyping nit above, this looks good to ship.
| networkCostsTooltipSuffix?: ReactNode | ||
| } | ||
|
|
||
| export function useTradeBasicConfirmDetailsData( |
There was a problem hiding this comment.
The file is probably should be in this PR. I don't see any places where it is used
There was a problem hiding this comment.
lol, it's from another pr, thank you for catch
| onErrorRetry: (_: unknown, __key, config, revalidate, { retryCount }) => { | ||
| onErrorRetry: (error: unknown, _key, config, revalidate, { retryCount }) => { | ||
| // Don't retry if error is "Unsupported chain" | ||
| if (error instanceof Error && error.message.toLowerCase().includes('unsupported chain')) { |
There was a problem hiding this comment.
Should isUnsupportedChainMessage be used here?
|
@limitofzero unfortunately, it doesn't fetch balances from BFF even when |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts (2)
13-24: Centralized BFF error parsing/handling looks good; consider exposing body message and hardening message extractionThe new flow around
BalanceResponse.message,parseBffResponse,handleBffError, andUnsupportedChainErroris a nice improvement and keeps unsupported-chain logic contained. A couple of small tweaks could make it more robust and easier to debug:
- In
handleBffError, you compute a richererrorMessagebut then drop it for the generic case. Using the parsed message in the thrown error would improve observability without changing behavior for callers:function handleBffError(res: Response, data: BalanceResponse | { message?: string }): never { const errorMessage = parseErrorResponse(data, res.statusText) if (isUnsupportedChainMessage(errorMessage)) { throw new UnsupportedChainError() } - throw new Error(`BFF error: ${res.status} ${res.statusText}`) + throw new Error(`BFF error: ${res.status} ${errorMessage}`) }
parseErrorResponsecurrently only recognizes an object with amessagefield; any other JSON shapes (e.g.{ error: 'Unsupported network id' }) or a string body will fall back tostatusText, which meansisUnsupportedChainMessagemay never see the real message. You could make this more resilient without affecting existing behavior:function parseErrorResponse(data: unknown, statusText: string): string { - if (typeof data === 'object' && data !== null && 'message' in data) { - return String((data as { message: unknown }).message) - } - return statusText + if (typeof data === 'string') { + return data + } + + if (typeof data === 'object' && data !== null) { + const anyData = data as any + if (anyData.message) return String(anyData.message) + if (anyData.error) return String(anyData.error) + } + + return statusText }
- Given the PR description mentions an
"Unsupported network Id"error from BFF whileisUnsupportedChainMessage(inUnsupportedChainError.ts) currently matches'unsupported chain', it’s worth double‑checking the actual BFF payload. If the phrase doesn’t include'unsupported chain',UnsupportedChainErrorwill never be thrown and the unsupported-chains tracking + SWR retry suppression won’t trigger. Consider broadening the matcher (e.g. to also cover'unsupported network'/'unsupported network id') once you confirm the BFF response text.Also applies to: 34-57, 153-165
65-69: Unsupported-chain effect wiring works; consider guarding chainId and narrowingisBffFailedsemanticsThe integration of
useIsBffSupportedNetworkanduseAddUnsupportedChainIdwithisUnsupportedChainError(error)nicely closes the loop between runtime BFF responses and future fetch gating.Two optional refinements you might consider:
- Be defensive about
targetChainIdwhen adding to the unsupported set. Today types say it’s always aSupportedChainId, but at runtime it’s still derived viachainId ?? activeChainId. A simple guard avoids ever storing an undefined inbffUnsupportedChainsAtomif this hook is misused in the future:useEffect(() => { const hasUnsupportedChainError = isUnsupportedChainError(error) - if (hasUnsupportedChainError) { - addUnsupportedChainId(targetChainId) - } - - setIsBffFailed(!!error) + if (hasUnsupportedChainError && targetChainId) { + addUnsupportedChainId(targetChainId) + } + + setIsBffFailed(!!error && !hasUnsupportedChainError) }, [error, setIsBffFailed, addUnsupportedChainId, targetChainId])
- Related: if the intended meaning of
isBffFailedAtomis “BFF infra is down/misbehaving”, you may not want to flag it for a clean “unsupported chain” response (which is more of a capability/configuration outcome). The adjustment in the diff above (!!error && !hasUnsupportedChainError) keeps failure semantics focused on genuine errors while unsupported networks are tracked exclusively viabffUnsupportedChainsAtom.These are UX/semantics tweaks rather than correctness issues, so feel free to skip if the current behavior matches existing UI expectations.
Also applies to: 94-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/balances-and-allowances/src/constants/bff-balances-swr-config.ts(2 hunks)libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts(4 hunks)libs/balances-and-allowances/src/utils/UnsupportedChainError.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/balances-and-allowances/src/constants/bff-balances-swr-config.ts
- libs/balances-and-allowances/src/utils/UnsupportedChainError.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/balances-and-allowances/src/hooks/usePersistBalancesFromBff.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/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts
🧬 Code graph analysis (1)
libs/balances-and-allowances/src/hooks/usePersistBalancesFromBff.ts (3)
libs/balances-and-allowances/src/utils/UnsupportedChainError.ts (3)
isUnsupportedChainMessage(12-14)UnsupportedChainError(1-6)isUnsupportedChainError(8-10)libs/balances-and-allowances/src/utils/isBffSupportedNetwork.ts (1)
useIsBffSupportedNetwork(14-17)libs/balances-and-allowances/src/state/isBffFailedAtom.ts (2)
useSetIsBffFailed(13-15)useAddUnsupportedChainId(19-32)
⏰ 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: Setup
- GitHub Check: Cypress
I think you wasn't able to check it due to setting for 10% users only
|
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @limitofzero , thank you, works like a charm besides some issues:
- could you please check whether it is possible to support Plasma here?
- Could you please check the rules for rate-limiting? I've started my tests, checked balances in all networks, and got blocked :(
- I noticed that balances are not reflected correctly/missing in all networks. The most vivd example is Base:
- It might be related to the previous case: after my swap and bridge trade the token's balance has not been updated in the dst chain
Could you please take a look at these issues?
Summary
Updated supported network list for bff (regarding migration to alchemy) and added disabling retries for "Unsupported network Id error"
To Test
pls make sure that the value for the flag is 100, otherwise you have a chance no to enable the api:
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.