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

Activate wallet-connect deeplink with qr-mode #650

Merged
merged 12 commits into from
Jun 23, 2021
Merged

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Jun 3, 2021

@henrypalacios henrypalacios marked this pull request as draft June 3, 2021 22:33
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

  • 🔭 IDO UI: Easy IDO auction

@@ -26,7 +26,7 @@ export const GlobalStyle = createGlobalStyle<{ theme: any }>`

export const ThemedGlobalStyle = createGlobalStyle`
html {
color: ${({ theme }) => theme.text1};
color: ${({ theme }) => theme.text4};
Copy link
Contributor Author

@henrypalacios henrypalacios Jun 5, 2021

Choose a reason for hiding this comment

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

walletconnect modal take the body>color style, I found no other way to change the text color, and it does not seem to affect anything, what do you think @alongoni?

Selection_305

@henrypalacios henrypalacios marked this pull request as ready for review June 5, 2021 00:17
@henrypalacios henrypalacios requested review from josojo and ramirotw June 5, 2021 00:17
Copy link
Contributor

@josojo josojo left a comment

Choose a reason for hiding this comment

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

Nice, really like tthis PR. Big improvement.

Well done!

@elena-zh
Copy link

elena-zh commented Jun 7, 2021

Hey @henrypalacios , @josojo
I have verified the issue. Great job! However, here are several notes:

  1. I have not been able to connect the app with the Safe app. See the video:
    https://drive.google.com/file/d/1LK8K4uXiIvfyYoouOYWwkpjlXOA7HXh_/view

  2. Sometimes the link is not copied: I see the message that it was copied, but when I paste it, I see the previously copied value
    https://drive.google.com/file/d/1aurDHyED-B2q2gyYkhXpenaTFotmi0bN/view

  3. Cross icon in the WalletConnect modal has double lines
    cross with double line
    image

  4. These issues might not be related to our current implementation but to the integrated component, however, I have to mention that:
    4.1. 404 page is opened when call connection in iOS using

  • Crypto.com
  • 1inch
  • CYBAVO
  • AlphaWallet
  • EasyPocket
  • SparkPoint
  • PEAKDEFI
  • Aktionariat
  • Flare
  • AToken
  • Tongue
  • RWallet
  • PlasmaPay
  • O3Wallet
  • Defiant
    4.2. Nothing happens, when the next apps are not installed in an iOS mobile device:
  • Pillar
  • Argent
  • ImToken
  • TokenPocket
  • Ledger
  • Dharma
  • Huobi
  • HaloDefi
  1. I'm not able to see app's list in my Android device:
    image

Could you please take a look into these issues?

Thanks

@henrypalacios
Copy link
Contributor Author

Thanks by your notes @elena-zh, with regard to the points discussed:

  • 1
    This surely had to do with the point 2.
  • 2
    Please try again.
  • 3
    This has to do with the modal that comes with the package, I don't know if it can be improved in any way, maybe @alongoni can suggest something.
  • 4
    I have not been able to reproduce this, but yes, surely this has to do with the walletconnect package.
  • 5
    I'm not sure if it looks different on iOS, but on Android after pressing connect, the possible options appears.
    Selection_279

@elena-zh
Copy link

elena-zh commented Jun 9, 2021

Hi @henrypalacios , great job!

  • Cases 1-2 are fixed.

  • I have created a separate issue for the case 3 - Cross icon in the WalletConnect modal has double lines #659 and assigned @alongoni to it.

  • As for the cases 4 and 5. In Android it works fine: the app suggests a user to open any supported app which can be connected using WalletConnect option.
    However, the issue I mentioned is still reproducible on iOS devices.
    @josojo , could you please take a look at the case 4 in my previous comment and clarify whether we should leave it as it is?
    The issue is that for several apps nothing happens when a user selects them in case if they are not installed to the device. Another case that a lot of apps navigate to the 404 page when select them.

  • Also, I have faced another issue that is related to this pr: walletconnect modal appears after disconnection of a wallet. And it is not closed when connect a wallet using it once again. @henrypalacios , could you please take a look at it?
    https://drive.google.com/file/d/1POb25DYAo20pKUplDAEVJmkVWA8uuY82/view

  • BTW, we have an low priority issue [connect a wallet modal] wrong header  #599 on the board related to a modal header name. Could you please fix it in this pr?

Thanks

@josojo
Copy link
Contributor

josojo commented Jun 9, 2021

  • @josojo , could you please take a look at the case 4 in my previous comment and clarify whether we should leave it as it is?
    The issue is that for several apps nothing happens when a user selects them in case if they are not installed to the device. Another case that a lot of apps navigate to the 404 page when select them.

Yes, this is fine, let's not change anything

@ramirotw
Copy link
Contributor

ramirotw commented Jun 9, 2021

Might be related to #600

@ramirotw
Copy link
Contributor

ramirotw commented Jun 9, 2021

Also, on a HD screen both the modal's header and the copy tooltip are not 100% visible

image

@henrypalacios
Copy link
Contributor Author

@elena-zh continuing your notes:

Also, I have faced another issue that is related to this pr: walletconnect modal appears after disconnection of a wallet. And it is not closed when connect a wallet using it once again. @henrypalacios , could you please take a look at it?

I have not been able to find a way to solve this, the problem is associated when the walletconnect modal appears immediately after disconnecting. The strange thing is that at times it works and at other times it does not. As in this video that occurs after the third attemp

Could you please fix it in this pr?

closes #599 also.

@ramirotw

Might be related to #600

@ramirotw It doesn't look like it because when refreshing walletconnect it is not really there (it is the InjectedConnector that remains).

Also, on a HD screen both the modal's header and the copy tooltip are not 100% visible.

Do you think we should open another issue for that, since I'm not sure if it's possible to just make a wrapper?

@elena-zh
Copy link

Hi @henrypalacios ,
#599 Looks good to me now!

However, today I have not been able to connect a wallet using walletconnect option: each time when I select this option, I get the app crash =(
image

As for the mentioned above issues, I have created tickets for them:
#670 - truncated modal header
#671 - Walletconnect modal appears after a wallet disconnection

Thanks

@henrypalacios
Copy link
Contributor Author

henrypalacios commented Jun 10, 2021

NoCloses 671 (failed)

@josojo
Copy link
Contributor

josojo commented Jun 11, 2021

@henrypalacios can you give a summary on what is working and what is not working?

Do you need help with anything particular? It would be great if we can get it ready for release.

@elena-zh
Copy link

@henrypalacios , @josojo ,
I have retested the PR. Here are my notes:

  1. Walletconnect modal appears again after a wallet disconnection #671 is still reproducible: connection window may appear again (50/50%) when I press on the 'Disconnect' button. I think, It worth fixing before the release due to connection does not work as expected when reconnect a wallet from the newly appeared modal window (see the issue description)
  2. Connection with Gnosis Safe works fine. I was able to run place/cancel/claim order transactions from safe, however, transaction status is not tracked: all transactions are in 'Pending' status in the GA app even if they are successfully completed: Claiming/placing/cancelling order process hangs when interact with Gnosis Safe #675. I think, we should fix it before the release.
    I have to notice, that transactions appropriately change their status when use walletconnect via barcode.
  3. still, there are several UI issues unfixed (On a HD screen both the modal's header and the copy tooltip are not 100% visible #670, Cross icon in the WalletConnect modal has double lines #659 ), but they are not high priority ones and can be fixed later.

Thanks

@elena-zh
Copy link

Hi @henrypalacios , @hpmaxi ,
the #671 issue seems to be fixed now.
As for the pending transactions issue, I cannot say that it is fixed. Now all 'pending' transactions do not appear at all: when I place an order, it is not displayed in the 'Your orders' section with 'pending' state.
Also, no confirmation pop-ups when a transaction ends. Then, nothing is displayed in the 'Your transactions' list even after reloading the app
https://drive.google.com/file/d/1alYBP2fdT47AqVqLLntj_n0bq2MRBtES/view

@hpmaxi
Copy link
Contributor

hpmaxi commented Jun 16, 2021

@elena-zh @josojo
I think this could be closed and make another issue with the safe related issues of the Elena's last comment. And close #671 too.

About safe, as far as I know, we are not updating the transactions state with the internal calls made by the safe, so maybe fixing that is completely unrelated to qr-mode.

[UPDATE]
@henrypalacios said that was able to reproduce #671 , but I think that could be made in another branch too.

@elena-zh
Copy link

Hi @hpmaxi , here a separate issue for Gnosis Safe transactions #693.
I was also able to reproduce #671 issue again - a reopened it and put it into the Review state. If we take a decision to fix it in another branch, it would be nice to unlink it from this PR and then move it into another column on the board.

Thanks

@josojo
Copy link
Contributor

josojo commented Jun 21, 2021

Yes, I think its fine to track them with issues and merge what we have already.

@henrypalacios
Copy link
Contributor Author

henrypalacios commented Jun 22, 2021

In this PR try to handle the error #658 when the app is crashed when connect a wallet with another unsupported network

@elena-zh
Copy link

elena-zh commented Jun 23, 2021

Hi @henrypalacios , thank you for the fix!
Now the app is not crashed, however, works a bit strange. Look:

  1. When I connect to another supported network, I receive an error message to change a network.

  2. A lot of errors in console appear right after connection
    here is the video: https://drive.google.com/file/d/10AGylZilk3K166nEHCnK2JbrZl_ae57r/view

It would be great to use the same flow as it works for Metamask connection:

@henrypalacios
Copy link
Contributor Author

Ok @elena-zh , I'm going to revert the changes related with #658 by merged this PR, and I will continue it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wallet connect deep link
5 participants