fix: (WC) do not return Safe as available on all optional Namespaces #3725

schmanu · 2024-05-16T09:42:22Z

What it solves

Sometimes the chainChanged event is ignored if the connected dapp did not acknowledge the new session yet.
Then the dapp is stuck with a wrong chain on which the Safe is not deployed.

How this PR fixes it

Only uses our workaround if the Safe is not present in the requiredNamespace

How to test it

Connect to uniswap with a polygon Safe
observe that the Safe is correctly recognized on Polygon

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 · 2024-05-16T09:42:36Z

Branch preview

✅ Deploy successful!

Website:
https://fix_wallet_connect_chainid--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://fix_wallet_connect_chainid--walletweb.review.5afe.dev/storybook/

github-actions · 2024-05-16T09:43:35Z

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 · 2024-05-16T09:46:09Z

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

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

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`1012.38 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!

src/features/walletconnect/services/WalletConnectWallet.ts

github-actions · 2024-05-16T09:48:32Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	79.26% (-0.02% 🔻)	11629/14672
🔴	Branches	58.14% (-0.04% 🔻)	2751/4732
🟡	Functions	66.33% (-0.04% 🔻)	1866/2813
🟢	Lines	80.62% (-0.02% 🔻)	10480/13000

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / index.tsx	82.35% (-8.82% 🔻)	66.67% (-16.67% 🔻)	50% (-12.5% 🔻)	83.87% (-9.68% 🔻)
🟢	`...` / WalletConnectWallet.ts	90.36% (-0.11% 🔻)	89.47% (-1% 🔻)	84%	92.41% (-0.09% 🔻)

Test suite run success

1468 tests passing in 202 suites.

Report generated by 🧪jest coverage report action from eec99d2

katspaugh

🦸

compojoom · 2024-05-16T11:34:46Z

src/features/walletconnect/services/WalletConnectWallet.ts

-      requiredChains.map(stripEip155Prefix),
-      optionalChains.map(stripEip155Prefix),
-    )
+    const supportedChainIds = [currentChainId].concat(requiredChains.map(stripEip155Prefix))


If we don't add the optional chains wouldn't that mean that the moment you switch your network the connection will be closed?

I will check

You are right. I will readd those :)

You can not return a chainId in the namespace without an account. So we would be stuck with the same workaround in the end which caused the bug.
I feel like having to reconnect after switching to another Safe is the lesser devil than not being able to connect on any chain other than mainnet. But We should discuss this.

francovenica · 2024-05-17T15:26:03Z

I got this alert/prompt when I tried to connect in Uniswap with Polygon which I don't think is intended

After accepting it (I wrote "Continue" but idk what to write because it doesn't say) didn't tirgger anymore, the only way to trigger it is by clearing the LS and cache of the safe app and trying again

Go to Uniswap, set it to ethereum there
Open the WC modal and copy the hash
Go to the safe app in a safe in Polygon
Paste the code in the WC modal to connect
The popUp should show up

francovenica · 2024-05-17T16:06:04Z

I'm having different behaviors between current prod, this PR and Uniswap and I don't know if they are intended.

In prod, you can easily change from one network to the other no problem, but if you try in this PR the Uniswap app will tell you that you wallet doesn't support the networks (tried also in prod CGW since it has all networks but with same result.

Although it might be considered an issue on Uniswap side, is weird that the behavior changes depending if we use the PR or current Prod
In prod:

Using this PR:

francovenica · 2024-05-21T06:12:10Z

I tried to connect to OpenSea several times. In the safe app you can see the connection (in the top bar) but in openSea, or the WC modal with the QR code doesn't even close or the WC option in the modal where you select the wallet shows a infinite loading icon.
I never got the message that OpenSea makes you sign to connect in the safe

I'm not don't know if this an issue from OpenSea side, but in current prod the connection works fine (the message pop's up, I signed, I connect just fine...)

schmanu · 2024-05-24T06:51:44Z

I got this alert/prompt when I tried to connect in Uniswap with Polygon which I don't think is intended

After accepting it (I wrote "Continue" but idk what to write because it doesn't say) didn't tirgger anymore, the only way to trigger it is by clearing the LS and cache of the safe app and trying again

Go to Uniswap, set it to ethereum there Open the WC modal and copy the hash Go to the safe app in a safe in Polygon Paste the code in the WC modal to connect The popUp should show up

@francovenica
I cannot reproduce this. Tried it multiple times for me the following happens, which is also expected and correct:

I open Uniswap on Ethereum Mainnet
I click on connect wallet via Wallet Connect and copy the hash
I go to our Safe Wallet interface and open a Safe on Polygon
Now I copy the hash into the WC Modal in that Safe
The Safe connects, no pop up shows and uniswap switches to Polygon

schmanu · 2024-05-24T06:53:24Z

I'm having different behaviors between current prod, this PR and Uniswap and I don't know if they are intended.

In prod, you can easily change from one network to the other no problem, but if you try in this PR the Uniswap app will tell you that you wallet doesn't support the networks (tried also in prod CGW since it has all networks but with same result.

Although it might be considered an issue on Uniswap side, is weird that the behavior changes depending if we use the PR or current Prod In prod:

This is expected and this is actually what this fix does:

In current Prod we pretend that the Safe exists on all networks, but in fact it only exists on Mainnet. Switching to a network where the Safe does not exist should not be offered by Uniswap

schmanu · 2024-05-24T06:56:43Z

I tried to connect to OpenSea several times. In the safe app you can see the connection (in the top bar) but in openSea, or the WC modal with the QR code doesn't even close or the WC option in the modal where you select the wallet shows a infinite loading icon. I never got the message that OpenSea makes you sign to connect in the safe

I'm not don't know if this an issue from OpenSea side, but in current prod the connection works fine (the message pop's up, I signed, I connect just fine...)

I also can not reproduce this:
I tried different Safes now on Mainnet, Optimism and Polygon and connection / signing the login message always worked without any issues :/

…inId

github-actions · 2024-05-24T12:55:36Z

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}

francovenica · 2024-05-24T14:41:39Z

Well, I tried to reproduce again the issues I reported, but they are working fine now

I cannot make that prompt to show up anymore
OpenSea is connecting fine now in different networks

I already tried Curve and Safe-as-owner and those work fine as well
Checked that chaning network in the safe app disconnects the current connected dapp in the WC topbar

LGTM

fix: do not return Safe as available on all optional Namespaces

c2e0225

katspaugh reviewed May 16, 2024

View reviewed changes

src/features/walletconnect/services/WalletConnectWallet.ts Outdated Show resolved Hide resolved

katspaugh changed the title ~~fix: do not return Safe as available on all optional Namespaces~~ fix: (WC) do not return Safe as available on all optional Namespaces May 16, 2024

schmanu added 2 commits May 16, 2024 11:48

tests: fix approve session tests

3b1a070

fix: rephrase comment

270c0e5

schmanu requested a review from katspaugh May 16, 2024 09:50

katspaugh approved these changes May 16, 2024

View reviewed changes

schmanu self-assigned this May 16, 2024

compojoom reviewed May 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/dev' into fix/wallet-connect-cha…

eec99d2

…inId

schmanu merged commit 433bb16 into dev May 24, 2024
14 checks passed

schmanu deleted the fix/wallet-connect-chainId branch May 24, 2024 15:41

github-actions bot locked and limited conversation to collaborators May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (WC) do not return Safe as available on all optional Namespaces #3725

fix: (WC) do not return Safe as available on all optional Namespaces #3725

schmanu commented May 16, 2024

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

katspaugh left a comment

compojoom May 16, 2024

schmanu May 16, 2024

schmanu May 16, 2024

schmanu May 16, 2024

francovenica commented May 17, 2024 •

edited

Loading

francovenica commented May 17, 2024

francovenica commented May 21, 2024

schmanu commented May 24, 2024

schmanu commented May 24, 2024

schmanu commented May 24, 2024

github-actions bot commented May 24, 2024

francovenica commented May 24, 2024

fix: (WC) do not return Safe as available on all optional Namespaces #3725

fix: (WC) do not return Safe as available on all optional Namespaces #3725

Conversation

schmanu commented May 16, 2024

What it solves

How this PR fixes it

How to test it

Checklist

github-actions bot commented May 16, 2024 • edited Loading

Branch preview

github-actions bot commented May 16, 2024 • edited Loading

ESLint Summary View Full Report

github-actions bot commented May 16, 2024 • edited Loading

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

⚠️ Global Bundle Size Increased

github-actions bot commented May 16, 2024 • edited Loading

Coverage report

Test suite run success

katspaugh left a comment

Choose a reason for hiding this comment

compojoom May 16, 2024

Choose a reason for hiding this comment

schmanu May 16, 2024

Choose a reason for hiding this comment

schmanu May 16, 2024

Choose a reason for hiding this comment

schmanu May 16, 2024

Choose a reason for hiding this comment

francovenica commented May 17, 2024 • edited Loading

francovenica commented May 17, 2024

francovenica commented May 21, 2024

schmanu commented May 24, 2024

schmanu commented May 24, 2024

schmanu commented May 24, 2024

github-actions bot commented May 24, 2024

ESLint Summary View Full Report

francovenica commented May 24, 2024

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

francovenica commented May 17, 2024 •

edited

Loading