Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: always switch wallet chain before signing/executing #3440

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Mar 14, 2024

What it solves

The Zerion Wallet mobile app, when connected via WalletConnect, connects to the wrong chain but onboard thinks it's on the right chain. In addition to that, onboard.setChain doesn't work for WalletConnect (probably because onboard thinks there's no need to switch the chain). Not sure on whose side is the bug, but this fixes it for us.

Basically, if we detect WalletConnect, it will always attempt to switch the chain even if it's seemingly already on the right one.

How to test

  1. Connect Zerion to Safe as a signer
  2. Attempt to sign or execute a tx
  3. On prod, it would show a "Safe contract not deployed on this network" error

Other cases:

  1. Switch MetaMask to some chain
  2. Connect it to a Safe on another chain
  3. Try to sign or execute a tx
  4. It should offer to switch the chain in MM and correctly sign afterwards

  1. Connect to MetaMask on the same chain as Safe
  2. It should sign/execute w/o switching the chain

  1. Connect some other WalletConnect wallet
  2. Try all signing/executing on different chains

Copy link

github-actions bot commented Mar 14, 2024

Copy link

github-actions bot commented Mar 14, 2024

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

Copy link

github-actions bot commented Mar 14, 2024

📦 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 1016.35 KB (🟡 +33 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!

Copy link

github-actions bot commented Mar 14, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
79.44% (+0.06% 🔼)
11131/14012
🔴 Branches
58.43% (+0.03% 🔼)
2612/4470
🟡 Functions
66.13% (+0.02% 🔼)
1796/2716
🟢 Lines
80.71% (+0.07% 🔼)
10026/12422

Test suite run success

1411 tests passing in 195 suites.

Report generated by 🧪jest coverage report action from 150f257

Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "could not decode result data" error is gone but I am still getting a "SafeProxy not deployed" error:

Screenshot 2024-03-14 at 17 25 22

It originates from the getUncheckedSafeSDK function where we call sdk.connect. I checked if maybe the chainIds don't match but they are all the same. Tested this with the 1.3.0 safe on mainnet from the slack request.

@katspaugh
Copy link
Member Author

@usame-algan what are the steps to reproduce this?

src/utils/wallets.ts Outdated Show resolved Hide resolved
@francovenica
Copy link
Contributor

Tested first on prod to see the issue. I noticed that during safe creation it doesn't check if it is in the current network or not.

In this PR it does and asks for the network to be changed to the current network the safe is in (the "change to NETWORK" button shows in the safe creation)
Zerion seems to have an "all network" option in its app, so during tx signing/execution I noticed that the change of network is automatic (in the top right corner you can see that the prefix changes on its own to the current network). I had no issues signing/executing txs
I tried with MM and trezor as well, siging/and executing while changing the network in the process. I had no issues with that either

LGTM

@katspaugh katspaugh merged commit 81c7a24 into dev Mar 20, 2024
14 checks passed
@katspaugh katspaugh deleted the switch-chain branch March 20, 2024 09:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants