fix(perps): centralize Arbitrum network check in deposit hooks to prevent missing network errors cp-7.72.0#27484
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…posit rejection test The test relied on the mock's initial factory implementation surviving jest.clearAllMocks(), which would break under resetAllMocks/restoreAllMocks.
|
ensureArbitrumNetworkExists should be called on the main screen, not when someone clicked buttons |
…n button click - Call ensureArbitrumNetworkExists in PerpsHomeView on focus (useFocusEffect) - Remove ensureArbitrumNetworkExists from usePerpsHomeActions (add funds/withdraw) - Remove from usePerpsNavigation.navigateToOrder, PerpsMarketDetailsView handleAddFunds, usePerpsBalanceTokenFilter, PerpsOrderRedirect - Update unit tests to match new flow and fix PerpsMarketDetailsView test syntax
| useFocusEffect( | ||
| useCallback(() => { | ||
| ensureArbitrumNetworkExists(); | ||
| }, [ensureArbitrumNetworkExists]), | ||
| ); |
There was a problem hiding this comment.
Why does this need to be in useFocusEffect? Wouldn't it make more sense to call this when we actually run business logic that requires arbitrum to exist?
Can you help me understand why we only need it when the component mounts?
There was a problem hiding this comment.
Should be resolved now
…etwork check in order redirect Add .catch() to ensureArbitrumNetworkExists in PerpsHomeView to prevent unhandled promise rejections when network addition fails. Restore the Arbitrum network existence check in PerpsOrderRedirect, which is reachable from Token Details without mounting PerpsHomeView.
… deposit methods Move ensureArbitrumNetworkExists into depositWithConfirmation and depositWithOrder in usePerpsTrading so every caller is automatically covered. Remove the now-redundant check from PerpsOrderRedirect.
…centralized network check usePerpsTrading now imports usePerpsNetworkManagement, which requires Redux context. Add direct-path mocks in usePerpsTrading, PerpsOrderView, and PerpsMarketDetailsView tests to prevent Invalid CAIP chain ID errors from transitive module loading.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
…ew tests The hook calls useNetworkEnablement which invokes parseCaipChainId, causing "Invalid CAIP chain ID" errors when useSelector returns unmocked values.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
This is a behavioral change in the Perps deposit flow - the timing of when Arbitrum network is ensured has changed. The Add Funds flow (deposit with confirmation) is the core Perps E2E test scenario. Tags selected:
Performance Test Selection: |
|
✅ E2E Fixture Validation — Schema is up to date |
|
abretonc7s
left a comment
There was a problem hiding this comment.
Review: PR #27484 — fix(perps): centralize Arbitrum network check in deposit hooks
Tier: full | Tests: all pass | TSC: clean | Device: validated on iOS sim with recipe
Correct centralization of ensureArbitrumNetworkExists() into depositWithConfirmation/depositWithOrder. All call sites covered. 1 suggestion, 2 nitpicks.
[suggestion] usePerpsTrading.test.ts:416
The depositWithConfirmation test verifies the controller was called but doesn't assert mockEnsureArbitrumNetworkExists was called. The central behavioral change of this PR — that the Arbitrum check now runs inside depositWithConfirmation — has no test coverage. If await ensureArbitrumNetworkExists() is deleted from usePerpsTrading.ts, all 289 tests still pass.
Suggested addition:
expect(mockEnsureArbitrumNetworkExists).toHaveBeenCalledTimes(1);Same pattern applies to the depositWithOrder test at line ~428 (currently not present in the test file — there is no depositWithOrder test). Both should assert the network check fires.
[nitpick] PerpsOrderView.test.tsx:297
Two mocks for usePerpsNetworkManagement are defined: one at the top-level jest.mock('../../hooks/usePerpsNetworkManagement', ...) (line 297) and one inside jest.mock('../../hooks', ...). Having both is defensive but adds noise — only one is needed depending on how the code imports it. Consider removing one.
Description
Ensures the Arbitrum network exists before every perps deposit transaction, preventing
No network client found for chainerrors when users haven't added Arbitrum to their wallet.Problem:
ensureArbitrumNetworkExists()was previously called at individual call sites (button handlers, redirect screens), making it easy to miss entry points. Notably,PerpsOrderRedirect— reachable from Token Details without mountingPerpsHomeView— had the check removed, causing deposits to fail when Arbitrum wasn't present. Additionally, theuseFocusEffectcall inPerpsHomeViewhad no.catch()handler, creating unhandled promise rejections when the network addition failed.Solution:
usePerpsTrading— bothdepositWithConfirmation()anddepositWithOrder()now callawait ensureArbitrumNetworkExists()before invoking the controller. Every caller is automatically covered.PerpsHomeView'suseFocusEffectwith a.catch()handler so the network is added before the user taps deposit, avoiding latency.PerpsOrderRedirect,usePerpsHomeActions(deposit and withdraw handlers), andusePerpsBalanceTokenFilter.usePerpsNetworkManagementwhere needed due to the new transitive dependency.Changelog
CHANGELOG entry: Fixed a bug where depositing into Perps from Token Details could fail if the Arbitrum network had not been added to the wallet
Related issues
Fixes: #28231
Fixes jira issue: https://consensyssoftware.atlassian.net/browse/TAT-2716
Fixes jira issue: https://consensyssoftware.atlassian.net/browse/TAT-2715
Manual testing steps
Screenshots/Recordings
N/A — no UI changes, logic-only fix.
Before
N/A
After
N/A
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Moves the Arbitrum network precondition into core perps deposit entrypoints, affecting all deposit flows; failures or timing changes could impact transaction creation and navigation to confirmations.
Overview
Centralizes Arbitrum network setup for deposits.
usePerpsTradingnow callsensureArbitrumNetworkExists()inside bothdepositWithConfirmationanddepositWithOrder, so every perps deposit path enforces the network prerequisite.Removes redundant pre-checks at call sites and hardens navigation timing. Callers like
PerpsOrderRedirect,usePerpsHomeActions, andusePerpsBalanceTokenFilterno longer manually gate on the network check;PerpsHomeViewkeeps a best-effort warm-up on focus with an added.catch()to avoid unhandled rejections, andPerpsMarketDetailsViewtriggersnavigateToConfirmationinside the deposit try/catch.Tests/presets updated for new transitive dependency. Multiple perps view/hook tests add or adjust mocks for
usePerpsNetworkManagement, addwaitForwhere async ordering changed, and the perps state preset now includes an Arbitrum network configuration to keep view tests stable.Written by Cursor Bugbot for commit 18ee027. This will update automatically on new commits. Configure here.