feat(wallet): use reown for wallets management#7639
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: |
d46928b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f126af42.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://fix-wallets.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
d46928b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4928a1a8.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://fix-wallets.swap-dev-5u6.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@libs/wallet/src/index.ts`:
- Around line 43-44: The file exports './api/state' twice in
libs/wallet/src/index.ts; remove the duplicate export line so there is only a
single "export * from './api/state'" entry to avoid redundant re-exports and
potential linter warnings.
In `@libs/wallet/src/updaters/WidgetStandaloneMode.updater.tsx`:
- Line 84: Fix the typo in the inline comment inside
WidgetStandaloneMode.updater (file: WidgetStandaloneMode.updater.tsx) by
changing "I dapp mode never connect to widget connector" to "In dapp mode, never
connect to widget connector" (or similar grammatically correct phrasing); update
the comment text where it appears in the WidgetStandaloneMode updater block to
include the leading "n" and add a comma after "mode" for clarity.
- Around line 51-58: The effect that runs on isDappMode currently calls
reownAppKit.disconnect and connectWalletById(COW_WIDGET_CONNECTOR_ID) every time
isDappMode flips to true; add a persistent ref guard (e.g.,
dappConnectAttemptedRef) checked inside the useEffect so the connectWalletById
call only runs the first time isDappMode becomes true, set the ref to true when
initiating the connection, and do not reset it when isDappMode goes false so
repeated toggles won’t trigger additional calls; update the useEffect
surrounding isDappMode, reownAppKit.disconnect, and connectWalletById to use
this ref guard.
- Around line 82-85: The comment above the WidgetStandaloneMode updater
contradicts the component docs and logic: update the comment to state that in
standalone mode the widget connector must never be used (i.e., we disallow
connecting to the widget connector), matching the main component documentation
and the implementation in the WidgetStandaloneMode updater.
In `@libs/wallet/src/wagmi/config.ts`:
- Around line 113-114: The call to connectWalletById(SAFE_CONNECTOR_ID) inside
the getIsSafeAppIframe() branch drops its returned Promise and can cause
unhandled rejections; update the code in the module that calls connectWalletById
to explicitly handle the Promise (either await it inside an async init function
or append .catch(...)) and surface/log the error instead of letting it bubble as
an unhandled rejection so failures during Safe iframe boot are handled
gracefully. Ensure you reference the existing getIsSafeAppIframe(),
connectWalletById(), and SAFE_CONNECTOR_ID symbols when adding the try/catch or
.catch handler.
- Line 57: Keep the widget connector included via connectors: getConnectors()
(the standalone hiding is already handled in WidgetStandaloneMode.updater.tsx by
filtering COW_WIDGET_CONNECTOR_ID on ConnectorController and calling
wagmiAdapter.syncConnections()), and fix the unhandled promise from
connectWalletById(SAFE_CONNECTOR_ID) by moving the auto-connect into an async
init handler (or an IIFE) and await it with try/catch (or attach .catch) to
log/handle errors instead of letting the promise reject unhandled; reference
getConnectors(), connectWalletById(SAFE_CONNECTOR_ID),
WidgetStandaloneMode.updater.tsx, ConnectorController, COW_WIDGET_CONNECTOR_ID,
and wagmiAdapter.syncConnections() when locating the related code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a211bf3-822d-4074-bdce-1372cffe30b5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/cowswap-frontend/package.jsonapps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsxapps/cowswap-frontend/src/modules/injectedWidget/index.tsapps/cowswap-frontend/src/modules/injectedWidget/updaters/WidgetStandaloneMode.updater.tsxapps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.tsapps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormButtons/tradeButtonsMap.tsxapps/cowswap-frontend/src/modules/tradeFormValidation/services/validateTradeForm.tsapps/cowswap-frontend/src/modules/tradeFormValidation/types.tsapps/cowswap-frontend/vite.config.mtslibs/wallet/package.jsonlibs/wallet/src/api/container/WalletProvider/index.tsxlibs/wallet/src/constants.tslibs/wallet/src/index.tslibs/wallet/src/reown/consts.tslibs/wallet/src/reown/init.tslibs/wallet/src/updaters/WidgetStandaloneMode.updater.test.tsxlibs/wallet/src/updaters/WidgetStandaloneMode.updater.tsxlibs/wallet/src/utils/connectWalletById.tslibs/wallet/src/utils/getIsSafeAppIframe.tslibs/wallet/src/wagmi/SafeConnectionHandler.tsxlibs/wallet/src/wagmi/Web3Provider.tsxlibs/wallet/src/wagmi/config.tslibs/wallet/src/wagmi/getConnectors.tslibs/wallet/src/wagmi/hooks/useDisconnectWallet.tslibs/wallet/src/wagmi/hooks/useIsRestoringConnection.tslibs/wallet/src/wagmi/initialReconnectLifecycle.tslibs/wallet/src/wagmi/providerIsolation.test.tslibs/wallet/src/wagmi/providerIsolation.tslibs/wallet/src/wagmiStorage.ts
💤 Files with no reviewable changes (8)
- libs/wallet/src/constants.ts
- apps/cowswap-frontend/src/modules/injectedWidget/index.ts
- libs/wallet/src/wagmi/providerIsolation.ts
- libs/wallet/src/reown/init.ts
- libs/wallet/src/wagmi/providerIsolation.test.ts
- libs/wallet/src/wagmi/initialReconnectLifecycle.ts
- libs/wallet/src/wagmi/SafeConnectionHandler.tsx
- apps/cowswap-frontend/src/modules/injectedWidget/updaters/WidgetStandaloneMode.updater.tsx
| }) | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Fixes @reown/appkit source maps, now you can debug it locally
|
Tests passed on both Wallet Connect and EOA. 1 Failed when disconnected on the following steps, which differ from prod:
In the widget however it works as expected 2 Widget starts with
|
|
|
||
| export const wagmiStorage = createStorage({ | ||
| key: WAGMI_STORAGE_KEY, | ||
| storage: localStorage, |
There was a problem hiding this comment.
I have a Claude comment regarding this change:
sessionStorage → localStorage: tab isolation is silently removed
wagmiStorage.ts now uses localStorage where the old code explicitly used sessionStorage. The old code had extensive comments explaining why sessionStorage was chosen: "Tab A stays on Rabby even if Tab B switches to MetaMask". Connecting in one tab now affects all tabs. If this is intentional (relying on AppKit's own isolation), it should be explicitly documented; if not, it's a regression.
I recall this being one issue the Bleu team spent a while fixing raised by Elena, as this was a regression given the current prod behaviour.
Is this change intentional?
There was a problem hiding this comment.
Yes, it was intentional.
wagmiStorage should have storage: localStorage to make the reconnect correct. Before, it was sessionStorage for multi-tab wallet isolation, but it didn't really work.
There was a problem hiding this comment.
Ok. If this behaviour cannot be restored, we better make @elena-zh aware as this will affect her tests.
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @shoom3301 , thank you!
Some issues:
- Safe swap feature: it shows 'connect wallet' button if I setup everything based on the instructions
- Widget dapp mode: it shows 'restoring wallet ' state when I'm disconnected:
- open the dapp mode
- connect to a wallet
- disconnect a wallet -->
Expected: show grayed out 'connect wallet'button
- same issue with the standalone mode :( .
To reproduce:
- make sure that the preview link for the CoW Swap app is not open!
- open widget configurator in the standalone mode
Expected result here: show an active 'connect wallet' button
- standalone mode: connection is not separated with the CoW Swap app (this issue was addressed in #7441)
- open the widget standalone mode
- open the cow swap app
- connect to wallet in the widget --> the app gets automatically connected
- disconnect a wallet in CoW Swap --> the app gets disconnected in the widget
Expected: connections should not interfere
5 CoW Swap app: connect wallet button may call this modal
Possible steps to reproduce (not always reproducible):
1 . connect to a wallet in the app
2. close the browser tab
3. reopen the browser tab
4. press on the 'connect wallet button after 'restoring wallet' message
|
@elena-zh thanks!
|
|
@elena-zh 2nd is not reproducible for me. Do you test it with |
|
@elena-zh nevermind, I've hardcoded |
That hardcodes to a given build, not to the PR url. You should use |
4 participants
Summary
After the hotfix, we realized that Safe app crashes. Debugging showed that it comes from
SafeConnectionHandler.While debugging I also realized that we have lots of redundant code and we don't utilize reown as expected.
Important
reconnectOnMount={false}to<WagmiProvider>, because Reown handles the reconnectwagmiStorageshould havestorage: localStorageto make the reconnect correct. Before, it wassessionStoragefor multi-tab wallet isolation, but it didn't really workWAGMI_STORAGE_KEYcovers isolation between CoW Swap/Widget/SafegetIsSafeAppIframe()helps us synchronously understand that the app is open in Safe, so we can configure the connector and initiate its connectionWhat was deleted (the old machinery)
What was added / changed
Auto-connects to Safe via connectWalletById(SAFE_CONNECTOR_ID) when in a Safe iframe. Exports wagmiAdapter, reownAppKit, wagmiStorage.
App-side wiring
In short: a refactor that trades a fragile custom wallet-lifecycle layer for Reown AppKit's native management, while adding a "Restoring wallet" UX state and a dev-tooling source-map fix.
To Test
CoW Swap
Safe
baseUrl: 'http://localhost:3000'inSwapWidgetof the appgetIsSafeAppIframeto always returntruein CoW SwapWidget standalone
Widget dappMode
Summary by CodeRabbit
New Features
Improvements
Chores