oceanprotocol / market

🧜‍♀️ THE Data Market
https://market.oceanprotocol.com
Apache License 2.0
193 stars 296 forks source link

Test walletconnect #1148

Closed mihaisc closed 2 years ago

mihaisc commented 2 years ago

Test if portis/walletconnect(+ ledger) are working in v4 for all the basic flows: publish (all 3 pricing schemas) , order, trade, add/remove liquidity

Previous issues with walletconnect : https://github.com/oceanprotocol/market/issues/592

LucaMilanese90 commented 2 years ago

I can't seem to be able to connect to the market with Portis, is it because https://v4.market.oceanprotocol.com/ is a preview link? Screenshot 2022-03-17 at 16 38 09

LucaMilanese90 commented 2 years ago

Also using WalletConnect and scanning the QR code does not work: with Metamask mobile (fails without giving any error message) and Rainbow mobile (fails with this error) image

LoznianuAnamaria commented 2 years ago

@kremalicious Can we add the Portis key?

kremalicious commented 2 years ago

Fun fact, 229 accounts seem to have used Portis in Ocean Market so far:

Screen Shot 2022-03-29 at 11 42 40
kremalicious commented 2 years ago

Alright, set the allow list domains to:

https://market.oceanprotocol.com,https://*.oceanprotocol.vercel.app,https://*.market.oceanprotocol.com
kremalicious commented 2 years ago

Regression of combination walletconnect + Ledger Live/any mobile wallet in v4 preview:

On iOS and iPadOS getting only the QR code after selecting walletconnect when it should bring up the mobile app screen to use it on same device, like on balancer:

image

We want this: image

mihaisc commented 2 years ago

Interesting last time i tested i explicitly remember that cool walletconnect select popup, i was kinda impressed. Not sure what happened , i don't think we touched the integration since then

LucaMilanese90 commented 2 years ago

I have been able to successfully publish an asset from a wallet connected through WalletConnect scanning the QR through Metamask Mobile. Correctly received the transaction notifications and sent the confirmation from my smartphone (iOS). https://v4.market.oceanprotocol.com/asset/did:op:6137dd7a9d068708df3b9527ebb3a82420c21c49eed36e28ffb067053839fa54

The connection works also with Rainbow mobile (through WalletConnect) but I was not able to select a testnet so I couldn't try if the publish flow works. The same applies to Portis: I can connect but I'm on Eth main net by default and don't see any option to switch. I noticed though that the nft image and infos are no longer displayed correctly on OpenSea.

kremalicious commented 2 years ago

great, thanks for testing!

I was not able to select a testnet

It's a bit hidden and weird interaction, but in Rainbow mobile in settings at very bottom is Developer Settings, there you can activate and switch to testnets.

Last time I used Portis there was also some setting somewhere to show testnets in the list.

I noticed though that the nft image and infos are no longer displayed correctly on OpenSea.

yes, was hoping this just goes away but seems not, started tracking this in #1330

LucaMilanese90 commented 2 years ago

@kremalicious Thanks for showing me how to add the testnets, I run some more tests with MetaMask mobile and all the functionalities are there and work as intended (pool, trade, publish, consume, history, switch network). I also tried to connect with Rainbow mobile through walletconnect, the connection is there. This time I'm connected to the correct testnet and I can see the balance, but anytime I have to confirm a transaction, the app correctly receives the notification but when I open it I see a loader that never resolves making impossible to proceed:

IMG_3322

Other functions like the button to switch to the correct network also don't work since the request never reach the smartphone (looks like the button is specific for the MetaMask client, maybe it should not be there at all if the connection is through some other wallet). For Portis, even though I'm able to establish the connection and the test networks are enabled, I still don't see a way to select a specific network, it always connect by default to ETH.

Screenshot 2022-04-12 at 10 00 05

kremalicious commented 2 years ago

Needs another test round as we updated the way we do signatures for download, and WalletConnect has more priority than Portis.

And double checked, Portis never implemented EVM network switching in the UI. Unless we create this UI and then trigger Portis SDK methods it can't be used in our market for anything else than ETH Mainnet. A network switcher is developed in #1331 but not meant as a universal solution yet. We basically would need to detect Portis and then modify all network switching logic to initiate a new Portis() for respective network.

mihaisc commented 2 years ago

I say we just drop portis support for now. We clearly don't have time to implement various wallet sdks.

kremalicious commented 2 years ago

I was about to kick out Portis in #1355, but then found a bunch of custom components commented out and just enabled them and this network switcher actually works:

Screen Shot 2022-04-25 at 12 53 52

Working meaning I can select Rinkeby, then click Show Portis and I see that my network changed on Portis. But app does not detect it although we have some handlePortisNetworkChange() in there which most likely has to be moved to our Web3 context provider. So if it's just this simple change to make it work we should run with it, otherwise we should kick it out as it would be only half-working.

As for Walletconnect, looks like some ad blockers are blocking registry.walletconnect.com, while walletconnect provider tries to reach that to construct its app list. So with latest package updates I have the app list again, on desktop & mobile:

Screen Shot 2022-04-25 at 13 16 43
LucaMilanese90 commented 2 years ago

I'm having troubles to download datasets after completing the purchase, at the signature time I always get this message (connected through walletconnect and metamask mobile). This happens both for pool and free assets, I couldn't test on fixed priced assets because I'm having this #1356 problem on basically all the available datasets Screenshot 2022-04-26 at 10 04 27

LucaMilanese90 commented 2 years ago

Also buying assets with pool prices is not working correctly:

I have no way of recording what's happening on the phone, but I'm basically confirming a bunch of transactions, on the video though you can see at 00:49 and 1:59 the price jumps and the download button disappearing (I'm hovering on top with the cursor but I haven't click on it) and at the end you see the failed signature.

https://user-images.githubusercontent.com/20192135/165255954-a1276264-c441-496c-9c40-047ef90b3e70.mov

LoznianuAnamaria commented 2 years ago

For now, let's remove Portis support and test more the WalletConnect

LucaMilanese90 commented 2 years ago

I'm having troubles to download datasets after completing the purchase, at the signature time I always get this message (connected through walletconnect and metamask mobile). This happens both for pool and free assets, I couldn't test on fixed priced assets because I'm having this #1356 problem on basically all the available datasets Screenshot 2022-04-26 at 10 04 27

On MM mobile connected through walletconnect I'm still having the same issue as of today: I can buy an asset but when it's time to download the operation fails with the same error. The same happens for a compute job, it fails at the moment of signing but this time the errors are only in the console Screenshot 2022-06-28 at 09 47 28

mihaisc commented 2 years ago

i think we can close this