fix: cannot connect with WC to Opensea

[compojoom]

If the user is trying to connect with WC to Opensea and the user is on a chain other than mainnet the first connection attempt fails. The second one succeeds. This is because when opensea sees the list of optional chains, after establishing a session it sends a request, but the request is for the optional chain and not for the required one and we end up blocking it. By giving up on the optional namespace Opensea sends the request to the requried chain.

How to test it

Go to opensea to a collection that is on let's say OP. Try to establish WC connection to your safe that is also on OP. Establishing a connection should work.

Screenshots

Checklist

• I've tested the branch on mobile 📱
• I've documented how it affects the analytics (if at all) 📊
• I've written a unit/e2e test for it (if applicable) 🧑‍💻

[github-actions]

Branch preview

✅ Deploy successful!

https://fix_wc_opensea--walletweb.review-wallet-web.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

[github-actions]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

• Result: ✅ success
• Annotations: 0 total

---

^{Report generated by eslint-plus-action}

[github-actions]

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

🎉 Global Bundle Size Decreased

Page	Size (compressed)
global	1016.32 KB (-1 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	79.38% (+0% 🔼)	11130/14021
🔴	Branches	58.39% (-0.01% 🔻)	2612/4473
🟡	Functions	66.03% (-0.07% 🔻)	1792/2714
🟢	Lines	80.64% (-0% 🔻)	10026/12433

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / index.tsx	100%	85% (-1.36% 🔻)	100%	100%
🟢	... / index.tsx	100%	95% (-0.56% 🔻)	100%	100%
🟢	... / index.tsx	93.02% (+0.72% 🔼)	88.24% (-3.43% 🔻)	76.92% (+1.92% 🔼)	94.74% (+0.62% 🔼)
🟢	... / index.tsx	86.67% (-13.33% 🔻)	66.67% (-33.33% 🔻)	33.33% (-66.67% 🔻)	85.71% (-14.29% 🔻)
🟢	... / index.tsx	83.33% (-1.85% 🔻)	100%	33.33% (-26.67% 🔻)	82.61% (-2.01% 🔻)

Test suite run success

1410 tests passing in 195 suites.

Report generated by 🧪jest coverage report action from 3c4a003

[compojoom]

@katspaugh - I've been thinking about the failing tests.
It's obviously expected that those 2 will fail as now I don't pass the optional namespace. This actually made me wonder whether the app will continue to work as expected. While reading the WC docs I realized that one can update the session with additional chains and accounts. So I thought - ok, so if we switch from let's say OP to ETH, we could pass the new account to WC and it would pass it down to the dapap. To my surprise we already do that :D So the dapp recognises that we are switching the account in the wallet and updates respectively. Now with my fix this no longer works, because we have an isUnsupportedChain check:

safe-wallet-web/src/features/walletconnect/services/WalletConnectWallet.ts

Line 150 in e25b770

return this.disconnectSession(session)

I've find another way to make this work. First we approve with requiredChain, then immediately after that we update the session with the Optional namespace. Then later our changes to the accounts will be properly propagated to all supported chains.

The test still fails though, but that is I guess expected as we are not really testing whether approve has succeeded, but rather whether it was called with the "correct" arguments :(

[compojoom]

ok, now it should be fine everywhere :(

[usame-algan]

Code looks good and we tested the happy path together (Connecting to OpenSea). I suggest we do regression tests for other WC cases before merging. Things like switching to a different Safe on the same network, switching to a different Safe on a different network etc.

[katspaugh]

👍

[francovenica]

LGTM

Connecting with a different network than the one expected by OpenSea, it changes the network in openSea itself and connects fine.