search

superhero-com / superhero-wallet

Superhero is a multi-blockchain wallet to manage crypto assets and navigate the web3 and DeFi space. Currently supporting Bitcoin, Ethereum and æternity blockchains.
https://wallet.superhero.com
ISC License
40 stars 38 forks source link

Rework Dapp Connection Screen #3340

Closed smaroudasunicorn closed 1 week ago

smaroudasunicorn commented 1 month ago

Below an image is presented about dapp connection

We need to rework dapp connection screen. This screen is about dapp requesting from the wallet to provide address of active account. Should not seem like a transaction.

Image

onvisions commented 1 month ago

Dapp Connection Screen

CASE 1. The dApp name can be fetched, the URL can be fetched, dApp favicon/logo can be fetched and displayed.

MVP Dapp Connection Template - Default

CASE 2. The dApp name can not be fetched, dApp favicon/logo can not be fetched and displayed so we are replacing it with the generic label "DAPP" and generic icon.

MVP Dapp Connection Template -Generic Dapp

CASE 3. Unknown URL! Rare case that can still occur. URL cannot be fetched by Superhero Wallet. In this case neither the favicon nor the dApp name will be available. Most probably it's a risky app with basically "unknown source". User should be warned about potential risks.

Tx Access, Sign, Raw tx (2)

Figma reference: https://www.figma.com/design/3oGLWzSH0oJljo4RETZtur/Superhero-Wallet-UI-(%E2%9C%94%EF%B8%8FUpdated)?node-id=37239-260033&node-type=frame&t=xkVASfrr0tXWPBQs-0

onvisions commented 1 month ago

@sotiris @CedrikNikita IMO there's an issue with the way we grant permission to third party dApp to view not only the current account but also all "current active accounts" in the future. In other words the dApp asks only once for permission to view current address but it will be able to view all "currently active addresses" until Superhero Wallet is connected. @CedrikNikita correct me if I'm wrong here.

I am concerned that the user is not aware what's happening. As a user I'd expect to give permission affecting only my current account address when connecting, logging in, providing permission to a dApp or platform.

I might want to use different accounts for different platforms and purposes. I would not expect to grant access to the dApp to view all my wallet account addresses and that's what we are currently doing. I think we should reconsider this behavior and ask for permission each time the currently active account is changed.

smaroudasunicorn commented 1 month ago

@onvisions dapp requests access to one address is not this the case?

onvisions commented 1 month ago

@onvisions dapp requests access to one address is not this the case?

As far as I understood this is not the case and that's why I raised my concerns from user perspective. Let's sync with @CedrikNikita to be on the same page if it's needed.

As far as the current task is concerned I have created the design according to request: the dApp is asking for permission to view user's currently active account address (the address that is active at the moment of granting the permission).

peronczyk commented 3 weeks ago

@onvisions , @smaroudasunicorn - I confirm that the DAPPs are actually connecting with the wallet, not the addresses. For AE if a DAPP is connected whenever user changes the active account the DAPP knows about that and can make an action based on this action. The best example is our testing DAPP: https://docs.aeternity.com/aepp-sdk-js/v13.3.2/examples/browser/aepp/

Try to connect and open the "Basic functionality" tab. Then open the extension and try to switch accounts. You'll see that the testing DAPP will update the active account on the fly.

This is why I agree with Tsvetan that this connect modal is not fully informative.

MetaMask is asking you to list the accounts you want to use for connection: Image

peronczyk commented 3 weeks ago

Dapp Connection Screen

CASE 1. The dApp name can be fetched, the URL can be fetched, dApp favicon/logo can be fetched and displayed.

MVP Dapp Connection Template - Default

I suggest to not put the dapp name (eg.: Superhero Dex) as a tag above the arrow. There is a limited space in this area and in my opinion putting the name there gives more risk of breaking the layout than a real value to the user.

Also regarding the icon: should we create our own list of icons we can display? I'm asking because the situation is the same as with the app browser: we are not able to obtain the dapp icon.

CASE 2. The dApp name can not be fetched, dApp favicon/logo can not be fetched and displayed so we are replacing it with the generic label "DAPP" and generic icon.

MVP Dapp Connection Template -Generic Dapp

The tags above the arrow are uppercased, so we are not able to display "dAPP". What do you think about using "DAPP"?

Also the "dApp" string displayed as the name is our own label which should be used when there is no name?

CASE 3. Unknown URL! Rare case that can still occur. URL cannot be fetched by Superhero Wallet. In this case neither the favicon nor the dApp name will be available. Most probably it's a risky app with basically "unknown source". User should be warned about potential risks.

Tx Access, Sign, Raw tx (2)

Not sure if this is possible. We always have the access to the URL of the app that tries to connect. @CedrikNikita am I right?

peronczyk commented 3 weeks ago

Also I see following issues in provided designs:

  1. I think reusing the transaction details header section is bad because it forces to display the DAPP data (name+address) in a narrow column which is problematic with long names and addresses.
  2. The dapp name and address are displayed twice: both in the header and in the area beneath it.

As we have 100% control on the connection modals we can display those screens in totally different way than the transaction details. What do you think about following structure: Image

This is a working POC done with the code and it didn't require much time to do. Obviously this is only a concept and can be further updated.

onvisions commented 3 weeks ago
  1. About duplicate info: image

I agree about this one we might get rid of: image

  1. About the fact we are currently not able to fetch the icon/favicon of any dApp - my suggestion will be to create and keep icons for a list of known and trusted dApps (like Superhero DEX) and use the generic dApp icon for all the rest.

  2. About the label my suggestion was to make an exception of the capitalized letters in this case and use dAPP instead of DAPP to emphasize it's a "decentralized app" (it's usually written in this way). However it won't be an issue to use DAPP if making an exception is a problem.

  3. About Case 3 (Unknown URL) - we can have such a case. The wording of the warning text (in yellow) is synced with @smaroudasunicorn

  4. About your suggestion to change the layout: @smaroudasunicorn if you think it's a better idea to use the suggested alternative layout (mockup provided by @peronczyk) I'll update the design accordingly. I was trying to utilize what we already have as visualization for transactions and smart contract calls.

smaroudasunicorn commented 3 weeks ago

I agree with Bart Comments

  1. I agree to change the layout of dapp connection instead of using the transaction modal one. Maybe to combine the above approaches above (keep the arrow i.e.)
  2. I agree about icons
  3. If we change the layout this will be removed. Even with this layout propose to remove the dAPP box. I agree to remove also DAPP name box.
  4. I think depends on the caller if i am not mistaken about the URL...
  5. As mentioned in the one, we could keep the visualization with the arrow and have something combined from Bart's proposal @onvisions
  6. Regarding account change, we can have it but i do not consider it as MVP and would like to avoid more time needed to deliver.
peronczyk commented 3 weeks ago

OK, so now I'm waiting for updated designs. Here's working POC of my changes: https://github.com/superhero-com/superhero-wallet/pull/3385

onvisions commented 2 weeks ago

@peronczyk @smaroudasunicorn Updated dApp connection template

1. dApp name can be fetched

1

2. dApp name can not be fetched

2

3. Trusted dApp

3

4. Unknown URL = unknown source

4

Figma reference: https://www.figma.com/design/3oGLWzSH0oJljo4RETZtur/Superhero-Wallet-UI-(%E2%9C%94%EF%B8%8FUpdated)?node-id=37239-260033&node-type=frame&t=FMM3evEbkmvY73q9-0

................................................................................

RESPONSIVE design for the Side panel (examples):

Dapp Connection 1

Dapp Connection 2

Dapp Connection 3

Figma reference (responsive, side panel): https://www.figma.com/design/3oGLWzSH0oJljo4RETZtur/Superhero-Wallet-UI-(%E2%9C%94%EF%B8%8FUpdated)?node-id=37493-273334&node-type=instance&t=rAy9jPPlzgGfAnOf-0

smaroudasunicorn commented 2 weeks ago

I am ok

peronczyk commented 1 week ago

The task is ready to be tested @Liubov-crypto

Regarding this part: Image we decided to use the dapp icon instead of the warning, but colored in orange.

Modals changed:

  1. Connect modal
  2. Share address list modal

Affected features:

  1. Account select items: Image - please test if nothing has changed.
Liubov-crypto commented 1 week ago
  1. I found some discrepancies between Figma design and wallet:
    • different text
    • different buttons title
    • ':' is present in wallet but missing in Figma design

ut

  1. As I understood we should show the short Dapp name, not to double url:

1

  1. Different modal for known Dapp:

3

  1. When I interacted with superhero dex there were some connection issues, I was disconnected and observed that our connection modal also has some changes. Is it expected in this case?

https://github.com/user-attachments/assets/b6f03b07-df92-48c5-a70b-6c36ce6e573e

the same for tokaen.org:

5

  1. When it opens as web wallet Propose Tx text is missing:

6

@peronczyk please check it.

Liubov-crypto commented 1 week ago

@onvisions @peronczyk One more question: do we decided to leave this screen as it is?

https://github.com/user-attachments/assets/aea4d0b2-51b2-441b-82c2-e3a62c50098c

onvisions commented 1 week ago

I agree with what is mentioned above by @Liubov-crypto. We can leave the ":" as implemented. The Wallet Connect is not affected by this changes and is not within the scope of the task as far as I know.

Regarding the permission to "propose transaction" - the suggested design is a template so it shows the way we should display multiple permissions if the dApp is requesting it. If dApp is requesting only to view current address we display only this. However can't we include this one as a general request in the template (aren't all the dApps requiring permission to propose transactions?)

image

onvisions commented 1 week ago

@peronczyk @Liubov-crypto 2 more remarks on my side:

  1. According to design the URL is always one line and horizontally scrollable with overflow hidden (so the user still can mark the text and see the whole URL or copy it in case they would like to). In this case check also the font style of the URL and dApp name. Seem a bit different in the implementation.

image

  1. From the screenshots I noticed that the account background is not matching the avatar as expected. Some issue with picking the right color may be.

image

peronczyk commented 1 week ago

All the UI related fixes are there.

@Liubov-crypto

  1. When I interacted with superhero dex there were some connection issues, I was disconnected and observed that our connection modal also has some changes. Is it expected in this case?

The disconnecting happens also on dev. But I don't fully understand this. The PR is about changing the connection screen.

  1. When it opens as web wallet Propose Tx text is missing:

This is how the dapp asks for permission. We are not controlling this. Can you compare with dev?

Liubov-crypto commented 1 week ago

All the UI related fixes are there.

@Liubov-crypto

  1. When I interacted with superhero dex there were some connection issues, I was disconnected and observed that our connection modal also has some changes. Is it expected in this case?

The disconnecting happens also on dev. But I don't fully understand this. The PR is about changing the connection screen.

  1. When it opens as web wallet Propose Tx text is missing:

This is how the dapp asks for permission. We are not controlling this. Can you compare with dev?

  1. I tested with dev and there was no disconnection issue for me. And today this branch also was working fine, without disconnection.

  2. Would we like to show superhero icon here? or leave it as it is?

su

also aescan:

Image

the rest is fixed, LGTM

peronczyk commented 1 week ago

Superhero.com and aescan added to trusted dapps. Can you retest @Liubov-crypto ?

Liubov-crypto commented 1 week ago

I found that Dapp name is missing in Permissions Settings:

https://github.com/user-attachments/assets/dbf1e544-420d-4046-b99f-ad5bce4102b1

Dev:

perm

@peronczyk please check it

peronczyk commented 1 week ago

Fixed @Liubov-crypto Please retest.

Liubov-crypto commented 1 week ago

LGTM