Updates wallet connection and chain switch flows to support wagmi#6899
Updates wallet connection and chain switch flows to support wagmi#6899bsvalverde wants to merge 86 commits intodevelopfrom
Conversation
…nitial-viem-wagmi-setup
…nitial-viem-wagmi-setup
…nitial-viem-wagmi-setup
…2-wallet-connection-eip-6963
|
@elena-zh I believe points 1 and 3 are fixed. Points 2, 4, 5, 6, 7, 8, 9 and 10 are related to the ON flag, so I believe the agreement is to test that later, is that correct? |
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/wagmi/Web3Provider.tsx (1)
1-30:⚠️ Potential issue | 🟡 MinorCall
appKit.close()on cleanup and add missing dependencies.Line 28 references
appKit.closewithout invoking it—this is a no-op and prevents intended cleanup. Wrap the initialization in a cleanup function that callsclose(), add explicit dependencies, and remove the TODO scaffolding before shipping.🛠️ Proposed fix
-useEffect(() => { - if (LAUNCH_DARKLY_VIEM_MIGRATION) { - // TODO M-7 COW-572 - // This will be moved back to config file on cleanup - const appKit = createAppKit(appKitParams) - - appKit.updateFeatures({ - connectorTypeOrder: ['recent', 'injected', 'featured', 'custom', 'external', 'recommended', 'walletConnect'], - }) - appKit.close - } -}, []) +useEffect(() => { + if (!LAUNCH_DARKLY_VIEM_MIGRATION) return + + const appKit = createAppKit(appKitParams) + appKit.updateFeatures({ + connectorTypeOrder: ['recent', 'injected', 'featured', 'custom', 'external', 'recommended', 'walletConnect'], + }) + + return () => { + appKit.close?.() + } +}, [LAUNCH_DARKLY_VIEM_MIGRATION, appKitParams])
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/legacy/state/application/hooks.ts (1)
30-40: Consider tracking migration work externally rather than inline scaffolding.The commented-out AppKit code and TODO references (M-7 COW-572) serve as placeholders for migration work. Per coding guidelines, TODO scaffolding should be resolved before shipping. Since this is a feature-flag migration in progress, consider:
- Tracking the re-enablement work in an issue/task system rather than inline comments
- When the AppKit path is re-enabled, address the toggle semantics concern from the prior review—
useAppKit().open()is open-only, not a toggle. You'll needuseAppKitState()to checkopenstate and callclose()appropriately, wrapped inuseCallbackfor referential stability.The current fallback behavior (returning
toggleModalfromuseToggleModal) is correct and stable.libs/wallet/src/wagmi/config.ts (1)
51-63: Prefer shared chain lists over re-deriving from enums.If a shared chain/network list exists in common constants, reuse it here to avoid drift between wagmi/network config and other modules.
As per coding guidelines, reuse chain/network lists from shared constants (
AFFILIATE_SUPPORTED_CHAIN_IDS, etc.) rather than cloning arrays.
…2-wallet-connection-eip-6963
Hey @bsvalverde , I believe so, I tested it according to the issue description you provided, so reported something in advance. As for the current fix, I still don't have possibility to check whether the flag is properly working as this Leadndro's comment has not been addressed (comes from this thread https://cowservices.slack.com/archives/C07FQTUKLJC/p1770129760752509?thread_ts=1770061770.456439&cid=C07FQTUKLJC. Sorry, in this link you should change the domain to your company internal's one in order to read it.
For now, I will focus on regression tests with the flag off, will let you know once I finish. Thanks. |
elena-zh
left a comment
There was a problem hiding this comment.
@bsvalverde , it appears that WC connection is not working in a desktop version.
It works in a mobile device, but not in the desktop app:
- Connect to Safe wallet with networks mismatch: the app shows 'disconnected' state, but it shows 'connected' state when I try to connect to a wallet once again
Connection is not reset when I refresh the page.
Connecting to EOA/another wallet where a chain matches:
- It connects, but the app immediately freezes
Summary
This PR updates wallet connection and network handling for the app, migrating all logic to the new Wagmi/Reown connection system while preserving feature parity with the existing Web3React-based system behind a feature flag. Key improvements include:
Video Demo:
https://www.loom.com/share/c79aadcc70d54b49a151c5f7276da9df
To Test
useViemMigrationtotrue.Summary by CodeRabbit
New Features
Configuration
Dependencies