Fix walletconnect double qr code updated #444

alfetopito · 2022-04-22T10:57:52Z

Summary

Same as #360 but updated to latest code on develop

Closes #351
Closes #342

github-actions · 2022-04-22T10:58:06Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-04-22T11:07:55Z

🔭 GP Swap: CoW Protocol v2 Swap UI

elena-zh · 2022-04-22T12:08:13Z

Hey @alfetopito , this PR, unfortunately, does not solve #342 issue.
See the video: https://watch.screencastify.com/v/ON2Cf4f1bALyZJkOCvux
Pending TX: https://rinkeby.etherscan.io/tx/0x2bedf11333683498b513b5c65f144ccca5655f01834643af3cdfda5b80faf0b1
Executed TX: 0x51bf829b6718dd7b295ea205d9316780185feccbbf2f257458e984315a51e764

As for the #352 bug, it is partially solved (case 1), but case 2 is still here:

alfetopito · 2022-04-22T14:39:55Z

@elena-zh the issue from the video seems to be related to the tx hash not being updated, correct?
Isn't that what Ramiro was trying to fix?

Does it work ok if you don't cancel/speed up txs?

As for #342 , didn't expect it to fix it. Will take a look at that in the other issue.

Lastly, does this PR addresses the issue mentioned with the double QR codes when trying to connect with WalletConnect?

elena-zh · 2022-04-22T14:50:31Z

@alfetopito , the video is about connection issues with Gnosis Safe transactions: 'View details' show a not existing TxHash, however, should navigate to the transactions list in Gnosis safe (described in #342 ), and it is not related to that what Ramiro was trying to fix.
I tested this issue here as @anxolin mentioned that it must be fixed in #360 pr in https://cowservices.slack.com/archives/C0361CDG8GP/p1650622134220669?thread_ts=1650619342.020279&cid=C0361CDG8GP
But according to your comments, the issue is not addressed here.

Lastly, does this PR addresses the issue mentioned with the double QR codes when trying to connect with WalletConnect?

Yes, as I have mentioned above, this issues fixes the 1st part of the #351 issue (double QR code).

alfetopito · 2022-04-22T16:03:46Z

@elena-zh now I see what you meant.

I've managed to get it working. Could you please try it out again with the Safe and other WalletConnect based wallets?

elena-zh · 2022-04-25T11:25:02Z

Hey @alfetopito , great changes!
#351 and #342 issues are fixed here and work great!

However, with the code in #360 PR the next issue was transferred to the current PR: gnosis/cowswap#2547 (comment)

The issue was reported about 1Inch wallet, but it is also reproducible for MM mobile wallet, so, from my perspective, it might be important to take a look and fix it.

So when I try to run wrap/unwrap/approve transactions in MM/1Inch wallets (using WC) in iOS, I get an additional pop-up to open the app, and then I'm navigated to the app store instead of being navigated to the wallet to sign the transaction

Could you please take a look into this issue?

Thanks!

elena-zh · 2022-04-26T11:35:49Z

The issue is not reproducible in Uniswap: https://drive.google.com/file/d/1xnmRWEFy9Ce8dYbkM8wowXyYlSCyr8Qn/view?usp=sharing

elena-zh · 2022-04-26T13:47:22Z

In Android it works through an intermediate step for selecting a wallet to sigh a transaction:

SVID_20220426_164509_1.mp4

W3stside · 2022-04-29T10:52:48Z

src/custom/hooks/useWalletInfo.ts

@@ -60,7 +60,8 @@ function checkIsSupportedWallet(params: {
 async function getWcPeerMetadata(connector: WalletConnectConnector): Promise<{ walletName?: string; icon?: string }> {
  const provider = (await connector.getProvider()) as WalletConnectProvider

-  const meta = provider.walletMeta
+  // fix for this https://github.com/gnosis/cowswap/issues/1929
+  const meta = provider.walletMeta || provider.signer.connection.wc.peerMeta


cant this just be provider.peerMeta? why the long chain in signer?

Also from Nenad's original PR.
Anyway, I'll update with your suggestion

W3stside · 2022-04-29T10:53:09Z

src/custom/components/WalletModal/WalletModalMod.tsx

+        .then(() => {
+          // Manually set the WalletConnectConnector http.connection.url to currently connected network url
+          // Fix for this https://github.com/gnosis/cowswap/issues/1930
+          if (connector instanceof WalletConnectConnector) {


can this also be connector.isWalletConnect?

W3stside · 2022-04-29T10:53:48Z

src/custom/hooks/web3.ts

@@ -88,7 +88,7 @@ export function useEagerConnect() {
    if (!active) {
      const latestProvider = localStorage.getItem(STORAGE_KEY_LAST_PROVIDER)

-      // if there is no last saved provider set tried state to true
+      // If there is no last saved provider set tried state to true


Not my changes.

For context: When reviewing Nenad's original PR, there were comments about the case of the comments.
Keep in mind these were all coming from Uniswap.
Anyway, @nenadV91 was very kind and did abide by the requests.

W3stside · 2022-04-29T10:54:15Z

src/custom/hooks/web3.ts

    window.addEventListener('beforeunload', handleBeforeUnload)

-    // remove beforeunload event listener on component unmount
+    // Remove beforeunload event listener on component unmount


is this some new grammarnazi lint rule or is this custom? seems very unnecessary

W3stside

approve with comments

W3stside · 2022-05-03T09:50:53Z

i have this PR open locally and running yarn install has added:

"@walletconnect/browser-utils@^1.7.8":
  version "1.7.8"
  resolved "https://registry.yarnpkg.com/@walletconnect/browser-utils/-/browser-utils-1.7.8.tgz#c9e27f69d838442d69ccf53cb38ffc3c554baee2"
  integrity sha512-iCL0XCWOZaABIc0lqA79Vyaybr3z26nt8mxiwvfrG8oaKUf5Y21Of4dj+wIXQ4Hhblre6SgDlU0Ffb39+1THOw==
  dependencies:
    "@walletconnect/safe-json" "1.0.0"
    "@walletconnect/types" "^1.7.8"
    "@walletconnect/window-getters" "1.0.0"
    "@walletconnect/window-metadata" "1.0.0"
    detect-browser "5.2.0"

"@walletconnect/client@^1.7.0":
  version "1.7.8"
  resolved "https://registry.yarnpkg.com/@walletconnect/client/-/client-1.7.8.tgz#62c2d7114e59495d90772ea8033831ceb29c6a78"
  integrity sha512-pBroM6jZAaUM0SoXJZg5U7aPTiU3ljQAw3Xh/i2pxFDeN/oPKao7husZ5rdxS5xuGSV6YpqqRb0RxW1IeoR2Pg==
  dependencies:
    "@walletconnect/core" "^1.7.8"
    "@walletconnect/iso-crypto" "^1.7.8"
    "@walletconnect/types" "^1.7.8"
    "@walletconnect/utils" "^1.7.8"

to my yarn lock file - the previous error @nenadV91 and @anxolin and I saw was referring to missing browser stuff. could be related

anxolin

This PR works for me in 1inch and IOS. Kind of, because it works only using ETHSIGN instead of EIP-712.

However, I don't see the reason why is the case.

As we see here, it tries v4, then v3 then ethsign:

I watched in the WS how the message that says that the method is not supported comes from the wallet. So I would think its 1inch not implementing some of this signing methods.
It's likely that their names changed a bit (EIP-712 rpc calls were changing names since it was in draft mode for a while). Maybe 1inch can review why the other wallets work, but not them?

I see that we changed the version of "ethers" (and the old one is no longer compatible without moding many parts of the project)

See the details of the relevant library versions here.
https://docs.google.com/document/d/1EXF-gE3ViJ_Yto-14Sy-BrLNZAT0K34EtyMihEWQi8k/edit#

I'm approving this PR so we don't delay more the version. After all, I was able to trade using 1inch IOS wallet, and probably the issue can be solved in the wallet side.

Some relevant conversations: https://cowservices.slack.com/archives/C0375NV72SC/p1651608634798579 and https://cowservices.slack.com/archives/C0361CDG8GP/p1651606102100549

- Force clearing another field. We might eventually need to do this with all fields - Removing some wait times

elena-zh · 2022-05-04T10:12:06Z

I still see a request to open App store when I send a request to Approve a token using MM

However, I proceeded and opened the AppStore, then opened MM from there, and was able to sign the approve transactions. Afterwards I was redirected back to the app store.
The same is for the warp/unwrap/convert transactions.
So it works somehow..
Here is the video: https://drive.google.com/file/d/1jR5AuVWL0O-PMRuIPdQod5uc7AGR-Hm4/view?usp=sharing

Placing/cancelling orders work fine

The same is for the 1Inch wallet. However, the CowSwap app page is changed to that one https://wallet.1inch.io/wc/, and transaction does not appear in the activity modal when go back to the Cowswap app.

alfetopito self-assigned this Apr 22, 2022

alfetopito requested review from a team April 22, 2022 10:58

alfetopito mentioned this pull request Apr 22, 2022

Fix walletconnect double qr code #360

Closed

elena-zh mentioned this pull request Apr 25, 2022

Uni merge 1.0.6 #441

Merged

ramirotw mentioned this pull request Apr 27, 2022

Integrate Order progress bar #454

Merged

elena-zh mentioned this pull request Apr 29, 2022

[342] Fix Gnosis Safe WalletConnect on uni merge not showing in activities panel properly #488

Closed

W3stside reviewed Apr 29, 2022

View reviewed changes

W3stside approved these changes Apr 29, 2022

View reviewed changes

anxolin changed the base branch from develop to release/1.14 May 3, 2022 12:19

alfetopito and others added 5 commits May 3, 2022 13:34

Fix wallet-connect double qr code

cef1a67

Fix for 1Inch and possibly some other wallets

5f2261a

Updated comments and disconnect error handling

80efa78

Forcing wallet connect connector type

509ded5

updated yarn lock

5755129

anxolin force-pushed the fix-walletconnect-double-qr-code_updated branch from 25d6909 to 5755129 Compare May 3, 2022 12:34

anxolin approved these changes May 3, 2022

View reviewed changes

Fix integ tests:

0e8fabb

- Force clearing another field. We might eventually need to do this with all fields - Removing some wait times

alfetopito merged commit dbbc606 into release/1.14 May 4, 2022

alfetopito deleted the fix-walletconnect-double-qr-code_updated branch May 4, 2022 10:05

github-actions bot locked and limited conversation to collaborators May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix walletconnect double qr code updated #444

Fix walletconnect double qr code updated #444

alfetopito commented Apr 22, 2022 •

edited

Loading

github-actions bot commented Apr 22, 2022

github-actions bot commented Apr 22, 2022

elena-zh commented Apr 22, 2022 •

edited

Loading

alfetopito commented Apr 22, 2022

elena-zh commented Apr 22, 2022

alfetopito commented Apr 22, 2022

elena-zh commented Apr 25, 2022

elena-zh commented Apr 26, 2022

elena-zh commented Apr 26, 2022

W3stside Apr 29, 2022

alfetopito Apr 29, 2022

W3stside Apr 29, 2022 •

edited

Loading

W3stside Apr 29, 2022

alfetopito Apr 29, 2022

W3stside Apr 29, 2022

W3stside left a comment

W3stside commented May 3, 2022

anxolin left a comment •

edited

Loading

elena-zh commented May 4, 2022

Fix walletconnect double qr code updated #444

Fix walletconnect double qr code updated #444

Conversation

alfetopito commented Apr 22, 2022 • edited Loading

Summary

github-actions bot commented Apr 22, 2022

github-actions bot commented Apr 22, 2022

elena-zh commented Apr 22, 2022 • edited Loading

alfetopito commented Apr 22, 2022

elena-zh commented Apr 22, 2022

alfetopito commented Apr 22, 2022

elena-zh commented Apr 25, 2022

elena-zh commented Apr 26, 2022

elena-zh commented Apr 26, 2022

W3stside Apr 29, 2022

Choose a reason for hiding this comment

alfetopito Apr 29, 2022

Choose a reason for hiding this comment

W3stside Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

W3stside Apr 29, 2022

Choose a reason for hiding this comment

alfetopito Apr 29, 2022

Choose a reason for hiding this comment

W3stside Apr 29, 2022

Choose a reason for hiding this comment

W3stside left a comment

Choose a reason for hiding this comment

W3stside commented May 3, 2022

anxolin left a comment • edited Loading

Choose a reason for hiding this comment

elena-zh commented May 4, 2022

alfetopito commented Apr 22, 2022 •

edited

Loading

elena-zh commented Apr 22, 2022 •

edited

Loading

W3stside Apr 29, 2022 •

edited

Loading

anxolin left a comment •

edited

Loading