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

WalletConnect transforms into Reown #1199

Merged
merged 18 commits into from
Oct 22, 2024
Merged

WalletConnect transforms into Reown #1199

merged 18 commits into from
Oct 22, 2024

Conversation

creeppak
Copy link
Member

@creeppak creeppak commented Oct 18, 2024

Summary

Replaced the WalletConnect integration with Reown. From UX perspective nothing really changed. If you look under the hood you'll notice it was updated both from Reown and our sides. This should improve stability, at least theoretically. We'll know this once we start testing it on different platforms and with different wallets.

This version is still likely to be unstable. It's already huge, so we're gonna fix bugs in a different PR.

Known Issues

  1. Wallet icons don't load.
  2. New Branded GUI should be applied.
  3. Can't switch to OP Sepolia Testnet when connected using MetaMask. Can switch to Mainnet and back to Sepolia however.
  4. Some wallets doesn't support test networks. Add the Eth Mainnet to the chains settings and set is as Primary in order to connect to these of wallets.

Fixed

  1. Chain switching is broken.
  2. System tries to open a deeplink for a new wallet even though was connected remotely using smartphone
    This creates an annoying bug, where file explorer opens up, whenever any input from user wallet is requested. What one has to do is just click on Unity editor to focus it.
  3. The system tries to redirect user to a wallet whenever the session is being restored. This operation doesn't require any input from the user.
  4. The system currently is not capable of identifying if a local or a remote wallet was connected.

Potential improvements

  • We should add a Reown configuration property for the redirection strategy, in case a remote wallet was connected and a confirmation is expected from the wallet app. Dev would want to notify user that he has to take action in his remote wallet to continue. We can show our info overlays by default.
  • Ping wallet when restoring session (there is a command I believe) to make sure the session can be safely restored, behave like there's no active connection otherwise.

Testing

All the platforms except WebGL should be tested to see how new integration behaves in different environments.

  • Desktop/Editor
  • Android
  • iOS

WebGL is not supported currently as it uses another approach even in Reown's AppKit. Adding support for it is a different task.

Please review this so we can merge it on Tuesday. All the fixes and improvements will be done in another PR, but don't let this stop you commenting on the code.

@creeppak creeppak self-assigned this Oct 18, 2024
@creeppak creeppak marked this pull request as ready for review October 21, 2024 15:15
Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

  • Not an actual issue, but annoying, in the editor when you click a sample that requires to sign a transaction (like Mint), I get a window error "The application can't be opened" but the popup appears in my phone and I can sign the tx and see the result in the console (this may happen only in Mac, but it didn't happen with WC and is not happing with web3auth or Hyperplay)
Screenshot 2024-10-21 at 1 28 18 PM
  • Wallets that appear in the editor list looks like they are for mobile, so they don't work in desktop
Screenshot 2024-10-21 at 1 31 56 PM
  • Not working in mobile, if a select a wallet provider, like Metamask, I get this error in the console:
NullReferenceException: Object reference not set to an instance of an object.
  at ChainSafe.Gaming.Reown.RedirectionHandler.GetWalletAppDeeplink (ChainSafe.Gaming.Reown.Models.WalletModel walletData) [0x00000] in <00000000000000000000000000000000>:0 
  at ChainSafe.Gaming.Reown.RedirectionHandler.BuildConnectionDeeplink (ChainSafe.Gaming.Reown.Models.WalletModel walletData, System.String wcUri) [0x00000] in <00000000000000000000000000000000>:0 
  at ChainSafe.Gaming.Reown.RedirectionHandler.RedirectConnection (System.String connectionUri, System.String walletId) [0x00000] in <00000000000000000000000000000000>:0 
  at ChainSafe.Gaming.Reown.Dialog.ConnectionDialogBase.OnLocalWalletButtonClick (System.String walletName) [0x00000] in <00000000000000000000000000000000>:0 
  at UnityEngine.Events.UnityEvent.Invoke () [0x00000] in <00000000000000000000000000000000>:0 
  at UnityEngine.EventSystems.ExecuteEvents.Execute[T] (UnityEngine.GameObject target, UnityEngine.EventSystems.BaseEventData eventData, UnityEngine.EventSystems.ExecuteEvents+EventFunction`1[T1] functor) [0x00000] in <00000000000000000000000000000000>:0 
  at UnityEngine.EventSystems.StandaloneInputModule.ProcessTouchPress (UnityEngine.EventSystems.PointerEventData pointerEvent, System.Boolean pressed, System.Boolean released) [0x00000] in <00000000000000000000000000000000>:0 
  at UnityEngine.EventSystems.StandaloneInputModule.ProcessTouchEvents () [0x00000] in <00000000000000000000000000000000>:0 
  at UnityEngine.EventSystems.StandaloneInputModule.Process () [0x00000] in <00000000000000000000000000000000>:0 
--- End of stack trace from previous location where exception was thrown ---

@creeppak
Copy link
Member Author

  • Wallets that appear in the editor list looks like they are for mobile, so they don't work in desktop

For some reason we can't filter wallets for desktop now, there is only iOS and Android in their new API. For all other cases it's just gonna show all the wallets available even if there is no client for desktop.

But I think I found a solution, gonna filter out the wallets than don't have the desktop link.

@creeppak
Copy link
Member Author

  • Not an actual issue, but annoying, in the editor when you click a sample that requires to sign a transaction (like Mint), I get a window error "The application can't be opened" but the popup appears in my phone and I can sign the tx and see the result in the console (this may happen only in Mac, but it didn't happen with WC and is not happing with web3auth or Hyperplay)

Hm.. I thought I fixes that one. So you connect with mobile, but it still tries to open something when a confirmation is expected from your wallet? Any other cases when it shows the error like that?

@creeppak
Copy link
Member Author

@juans-chainsafe Oh snap! I simply forgot to push 🥲Please check it again now as I pushed it.

Copy link
Collaborator

@rob1997 rob1997 left a comment

Choose a reason for hiding this comment

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

Nice work!! left a few comments

# Conflicts:
#	Packages/io.chainsafe.web3-unity.hyperplay/Runtime/Libraries/ChainSafe.Gaming.HyperPlay.dll
#	Packages/io.chainsafe.web3-unity.lootboxes/Chainlink/Runtime/Libraries/ChainSafe.Gaming.Lootboxes.Chainlink.dll
#	Packages/io.chainsafe.web3-unity.lootboxes/Chainlink/Runtime/Libraries/Chainsafe.Gaming.Chainlink.dll
#	Packages/io.chainsafe.web3-unity.mud/Runtime/Libraries/ChainSafe.Gaming.Mud.dll
# Conflicts:
#	scripts/data/published_dependencies.txt
Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

The previous 3 issues were fixed!

I have 2 more things at the moment (I still need to try with mainnet in other wallet providers to see how it goes in iOS)

  • In desktop, I only see these 6 providers, before we could scroll down a lot more, is this on purpose?
Screenshot 2024-10-22 at 9 46 52 AM
  • In mobile, after you connect Metamask and you click a sample that requires to sign a transaction, you get redirected to Metamask, sign the transaction, go back to the Unity app and the loading spinner is there forever, even when the transaction finishes. And it also happens with simple things like Sign Message sample (let me know if you need a video for this)

@creeppak
Copy link
Member Author

  • In desktop, I only see these 6 providers, before we could scroll down a lot more, is this on purpose?
Screenshot 2024-10-22 at 9 46 52 AM

This is the only wallets supported by Reown that are available on desktop. I guess all the old ones we had didn't work anyway, so now we only show those that are 'officialy' supported by Reown on desktop.

@creeppak
Copy link
Member Author

  • In mobile, after you connect Metamask and you click a sample that requires to sign a transaction, you get redirected to Metamask, sign the transaction, go back to the Unity app and the loading spinner is there forever, even when the transaction finishes. And it also happens with simple things like Sign Message sample (let me know if you need a video for this)

That's weird, never experienced anything similar. Could you please create an issue with steps to reproduce?

@kantagara kantagara added the ready-to-merge Ready to Merge PR - this'll trigger required checks label Oct 22, 2024
@juans-chainsafe
Copy link
Contributor

  • In mobile, after you connect Metamask and you click a sample that requires to sign a transaction, you get redirected to Metamask, sign the transaction, go back to the Unity app and the loading spinner is there forever, even when the transaction finishes. And it also happens with simple things like Sign Message sample (let me know if you need a video for this)

That's weird, never experienced anything similar. Could you please create an issue with steps to reproduce?

#1201

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

The current status of the wallets that you can connect is awesome!
connect wallet status in iOS:
Metamask ✅
Trust Wallet ✅
Zerion ✅
Ledger - I can’t connect it right now, my ledger is not configured in the iphone
Rainbow Wallet ✅
Exodus ✅
MyEtherWallet - doesnt have a mobile app
ZenGo - is not sending me the email to login, so I can’t test it right now
Phantom Wallet ❌ Doesn’t appear in the WC list
Coinbase wallet ❌ is not on the list

@creeppak creeppak added ready-to-merge Ready to Merge PR - this'll trigger required checks and removed ready-to-merge Ready to Merge PR - this'll trigger required checks labels Oct 22, 2024
@creeppak creeppak added ready-to-merge Ready to Merge PR - this'll trigger required checks and removed ready-to-merge Ready to Merge PR - this'll trigger required checks labels Oct 22, 2024
@creeppak creeppak added ready-to-merge Ready to Merge PR - this'll trigger required checks and removed ready-to-merge Ready to Merge PR - this'll trigger required checks labels Oct 22, 2024
@creeppak creeppak merged commit bad25d0 into dev Oct 22, 2024
9 checks passed
@creeppak creeppak deleted the oleksandr/reown branch October 22, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to Merge PR - this'll trigger required checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants