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

Feat: native WalletConnect #2535

Merged
merged 136 commits into from
Oct 30, 2023
Merged

Feat: native WalletConnect #2535

merged 136 commits into from
Oct 30, 2023

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Sep 21, 2023

WalletConnect is now a first-class citizen in the app.

  • most dapps will now work thanks to dynamic chain switching
  • always available from the top toolbar
  • supports multiple connections at the same time
  • shows security warnings for risky dapps and bridges that don't play well with smart contract wallets
  • supports ?wc= query param in the URL
Screenshot 2023-10-23 at 16 24 03 Screenshot 2023-10-23 at 16 24 37

Specs: https://www.notion.so/safe-global/WalletConnect-as-first-class-citizen-c8578e20f9ac464e9c6f8cb3a973193b?pvs=4

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Branch preview

✅ Deploy successful!

https://nect--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

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

@katspaugh katspaugh force-pushed the nect branch 2 times, most recently from 8273d8e to 99fb684 Compare September 23, 2023 07:48
@francovenica
Copy link
Contributor

minor issue: we should put a more friendly error message if the user adds a random string of characters in the input.
image

katspaugh and others added 3 commits October 25, 2023 16:35
* Feat: add extra tracking events

* Fix tests

* Fix: debounce input error tracking
* Refactor: on close callback for WalletConnect

* Use safeAddress + chainId instead of raw query

* Close modal on path change
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

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
Copy link
Contributor

francovenica commented Oct 27, 2023

This ticket will serve as the RC, so issues found during regression will be listed here:

1 - When signing a tx, the alert that " you will leave the tx form " shows. Even tho signing will take you out of the tx form as expected is also expected that the alert doesn't show up:
Create a tx
Sign with your wallet
See the Alert popping up

Queue a tx
Then reject it
When clicking the Reject tx button the alert shows up

regression 1

@katspaugh
Copy link
Member Author

Fixed, @francovenica! ✅

@francovenica
Copy link
Contributor

It is fixed for the regular send funds modal.

It still happens when you have the "Replace transaction" modal open. clicking any of the buttons "Send token" or "Reject transaction" also triggers the alert
image

@katspaugh katspaugh force-pushed the nect branch 3 times, most recently from bb76267 to a011fe4 Compare October 29, 2023 15:25
@francovenica
Copy link
Contributor

francovenica commented Oct 30, 2023

The issues with the alert were fixed

I tried again 1inch, the issue I had with the tx not popping up and now it does pop up.

The regression went fine:
Connection with different wallet, with tx creation an execution,
Signing with policies 1/x and 2/x
Adding/removing/replacing tx
Safe creations
Send funds tx
Reject tx
App tx (tx builder with dummy contracts)
Spending limits creation and usage
Bulk tx execution
Batch tx list
Address book
Import/export
.....

Also tested "Safe as owner" with the new feature. Works fine

I tried a few more dapps (cowswap, Yearn, balance) with a Eth safe 1/x with with those work fine

Still have the issue of some dapps remaining in a pending status after the tx was fully executed in the safe. I still not sure if those are from the dapps side or ours.

I think is impossible to know if every dapp outhere will work fine or not, so let's make sure after the deployment that the feature toggle works fine.

@katspaugh katspaugh merged commit 458d7e0 into dev Oct 30, 2023
8 of 9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2023
@katspaugh katspaugh deleted the nect branch December 13, 2023 08:25
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.

4 participants