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

Feat/extension popup #82

Merged
merged 5 commits into from
Jun 5, 2024

Conversation

franfernandez20
Copy link
Contributor

@franfernandez20 franfernandez20 commented Mar 14, 2024

Description:
This pull request offers a solution to automatically open a Chrome extension when pairing a new session or sending a request from a DApp to a Wallet.

  • Add a new parameter to DAppConnector to select the supported extensions.
  • Add a method for detecting the installed extensions.
  • Add a method to pair with a single extension using its ID, without a QR or pairing string.
  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@franfernandez20 franfernandez20 marked this pull request as draft March 14, 2024 22:26
@franfernandez20 franfernandez20 force-pushed the feat/extension-popup branch 2 times, most recently from 8d5df93 to 1d59e10 Compare March 19, 2024 10:58
@franfernandez20 franfernandez20 marked this pull request as ready for review March 19, 2024 10:59
@franfernandez20
Copy link
Contributor Author

By default, it is not possible to directly pop up an extension with Wallet Connect. However, to
allow this possibility, the dAppConnector accepts a list of extension IDs. If you create the
AppConnector with an extension ID, it will automatically send a message to the extension to
detect if it is installed. In case the extension is installed, it will be added to the available
extensions and its data can be found at the extensions property of dAppConnector.

To connect an available extension, use the method connectExtension(<extensionId>). This will
link the extension to the signer and session. Whenever you use the signer created for this
session, the extension will automatically open. You can find out if the extension is available
by checking the extensions property.

const dAppConnector = new DAppConnector(
  dAppMetadata,
  LedgerId.TESTNET,
  projectId,
  Object.values(HederaJsonRpcMethod),
  [HederaSessionEvent.ChainChanged, HederaSessionEvent.AccountsChanged],
  [HederaChainId.Testnet],
  ['<Extension ID 1>, <Extension ID 2>'],
)

[...]

dAppConnector?.extensions?.forEach((extension) => {
  console.log(extension)
})

const extension = dAppConnector?.extensions?.find((extension) => extension.name === '<Extension name>')
if (extension.available) {
  await dAppConnector!.connectExtension(extension.id);
  const signer = dAppConnector.getSigner(AccountId.fromString('0.0.12345'))

  // This request will open the extension
  const response = await signer.signAndExecuteTransaction(transaction)
}

Wallets that are compatible should be able to receive and respond to the following messages:

  • "hedera-extension-query-<extesnionId>": The extension is required to respond with
    "hedera-extension-response" and provide the next set of data in the metadata property.
    let metadata = {
      id: '<extesnionId>',
      name: '<Wallet name>',
      url: '<Wallet url>',
      icon: '<Wallet con>',
      description: '<Wallet url>',
    }
  • "hedera-extension-open-<extensionId>": The extension needs to listen to this message and
    automatically open.
  • "hedera-extension-connect-<extesnionId>": The extension must listen to this message and
    utilize the pairingString property in order to establish a connection.

This communication protocol between the wallet and web dApps requires an intermediate script to
use the Chrome API. Refer to the
Chrome Extensions documentation

@teacoat
Copy link
Contributor

teacoat commented Mar 19, 2024

It may be worth considering adding something in here for dapp browser pairing, since it sort of deals with the same sort of postMessage stuff, here is a snippet that we use:

window.parent.postMessage(
            {
                type: "hashconnect-iframe-pairing",
                pairingString: this._pairingString,
            },
            "*"
        );

@franfernandez20
Copy link
Contributor Author

It may be worth considering adding something in here for dapp browser pairing, since it sort of deals with the same sort of postMessage stuff, here is a snippet that we use:

window.parent.postMessage(
            {
                type: "hashconnect-iframe-pairing",
                pairingString: this._pairingString,
            },
            "*"
        );

Definitely, @teacoat , that's a good point. I will prepare a proposal.

@franfernandez20 franfernandez20 force-pushed the feat/extension-popup branch 2 times, most recently from ee68ef7 to 601e71a Compare March 21, 2024 14:48
@teacoat
Copy link
Contributor

teacoat commented Mar 22, 2024

My only other feedback on this would be how does this play with the walletconnect modal wallet list, is it reinventing the wheel if there is already a way to deeplink into extensions? I'm not super familiar with the modal, but it has been on my list to investigate what the 'walletconnect way' of doing this is.

@franfernandez20
Copy link
Contributor Author

My only other feedback on this would be how does this play with the walletconnect modal wallet list, is it reinventing the wheel if there is already a way to deeplink into extensions? I'm not super familiar with the modal, but it has been on my list to investigate what the 'walletconnect way' of doing this is.

The WalletConnect modal doesn't let you pop up an extension, at least I didn't find some way to do it. As you say it is implemented with deeplinks, which means that on desktop, it redirects to another web page. To address this issue, one possible solution for dApps could be to display only the WalletConnect modal on mobile versions, while showing one button per extension along with the WalletConnect button on desktop.

@justynspooner
Copy link
Contributor

My only other feedback on this would be how does this play with the walletconnect modal wallet list, is it reinventing the wheel if there is already a way to deeplink into extensions? I'm not super familiar with the modal, but it has been on my list to investigate what the 'walletconnect way' of doing this is.

The WalletConnect modal doesn't let you pop up an extension, at least I didn't find some way to do it. As you say it is implemented with deeplinks, which means that on desktop, it redirects to another web page. To address this issue, one possible solution for dApps could be to display only the WalletConnect modal on mobile versions, while showing one button per extension along with the WalletConnect button on desktop.

WalletConnect appear to be pushing the Web3Modal which does support injected extensions as per EIP-6963

https://docs.walletconnect.com/web3modal/about

It might be worth reviewing this for the front end solution to support wallet browser extensions.

@justynspooner
Copy link
Contributor

After reading more into this I think we should consider proposing that Hedera wallets add support for EIP-6963. This would allow WalletConnect Web3Modal via Wagmi Multi Injected Provider Discovery to list injected wallets very similar to what we're proposing here in this PR just in an already recognised standard way.

@franfernandez20
Copy link
Contributor Author

My only other feedback on this would be how does this play with the walletconnect modal wallet list, is it reinventing the wheel if there is already a way to deeplink into extensions? I'm not super familiar with the modal, but it has been on my list to investigate what the 'walletconnect way' of doing this is.

The WalletConnect modal doesn't let you pop up an extension, at least I didn't find some way to do it. As you say it is implemented with deeplinks, which means that on desktop, it redirects to another web page. To address this issue, one possible solution for dApps could be to display only the WalletConnect modal on mobile versions, while showing one button per extension along with the WalletConnect button on desktop.

WalletConnect appear to be pushing the Web3Modal which does support injected extensions as per EIP-6963

https://docs.walletconnect.com/web3modal/about

It might be worth reviewing this for the front end solution to support wallet browser extensions.

As I can understand and what I get from WalletConnect team, is that Web3Modal is not compatible with signClient which we are using to make the communication. It is designed to use Wagmi methods to sign what I think is not ready for Hedera yet. But I agree that is a good solution to investigate and try to progress.

@franfernandez20
Copy link
Contributor Author

After reading more into this I think we should consider proposing that Hedera wallets add support for EIP-6963. This would allow WalletConnect Web3Modal via Wagmi Multi Injected Provider Discovery to list injected wallets very similar to what we're proposing here in this PR just in an already recognised standard way.

Definitely, it's best to adhere to this standard instead of creating something new from scratch. However, I am uncertain about whether it is compatible with Wagmi. I will investigate integrating EIP-6963 into the pop-up extension functionality. Nonetheless, I think this PR can be a starting point for short-term use.

@Volind
Copy link

Volind commented Mar 29, 2024

The most easiest way for now that will not require DApps to add support for every extension manually and will reuse what WalletConnect modal is proposing, is to use Desktop listing for your project and setup a website URL that content scripts of the extension will monitor and search for /wc?uri=.... Then extension can extract URI from search parameters and open UI with session proposal.

@AdrianKBL
Copy link

The most easiest way for now that will not require DApps to add support for every extension manually and will reuse what WalletConnect modal is proposing, is to use Desktop listing for your project and setup a website URL that content scripts of the extension will monitor and search for /wc?uri=.... Then extension can extract URI from search parameters and open UI with session proposal.

I truly believe that this isn't the way forward. The approach you're suggesting would result in a very poor user experience. Imagine every time I try to connect to a DApp, it opens a new webpage prompting me to connect an extension. Then, once I approve the connection, I have to close that webpage and go back to the previous one. I don't think this is the way to attract a mainstream audience to Hedera. A typical Web2 user would likely close the new webpage immediately, fearing it's a scam.

@Volind
Copy link

Volind commented Mar 30, 2024

The most easiest way for now that will not require DApps to add support for every extension manually and will reuse what WalletConnect modal is proposing, is to use Desktop listing for your project and setup a website URL that content scripts of the extension will monitor and search for /wc?uri=.... Then extension can extract URI from search parameters and open UI with session proposal.

I truly believe that this isn't the way forward. The approach you're suggesting would result in a very poor user experience. Imagine every time I try to connect to a DApp, it opens a new webpage prompting me to connect an extension. Then, once I approve the connection, I have to close that webpage and go back to the previous one. I don't think this is the way to attract a mainstream audience to Hedera. A typical Web2 user would likely close the new webpage immediately, fearing it's a scam.

Proposed in PR approach is also not flexible and doesn't leverage all power of WalletConnect. With extension you can easily close tab after opening extension that's not an issue at all. So it will blink for a while but this will be a way to propose connection through the modal itself where user will see all available wallets and dApps will not need to add support for new wallets. So everything will be managed by WalletConnect. Yes, having an injected provider could be another way of solving extension connection issue but we had an issue previously when trying to integrate it. I would suggest follow with Desktop listing approach through Modal or EIP-6963 if it will be possible to implement for Hedera.

@franfernandez20
Copy link
Contributor Author

The most easiest way for now that will not require DApps to add support for every extension manually and will reuse what WalletConnect modal is proposing, is to use Desktop listing for your project and setup a website URL that content scripts of the extension will monitor and search for /wc?uri=.... Then extension can extract URI from search parameters and open UI with session proposal.

I truly believe that this isn't the way forward. The approach you're suggesting would result in a very poor user experience. Imagine every time I try to connect to a DApp, it opens a new webpage prompting me to connect an extension. Then, once I approve the connection, I have to close that webpage and go back to the previous one. I don't think this is the way to attract a mainstream audience to Hedera. A typical Web2 user would likely close the new webpage immediately, fearing it's a scam.

Proposed in PR approach is also not flexible and doesn't leverage all power of WalletConnect. With extension you can easily close tab after opening extension that's not an issue at all. So it will blink for a while but this will be a way to propose connection through the modal itself where user will see all available wallets and dApps will not need to add support for new wallets. So everything will be managed by WalletConnect. Yes, having an injected provider could be another way of solving extension connection issue but we had an issue previously when trying to integrate it. I would suggest follow with Desktop listing approach through Modal or EIP-6963 if it will be possible to implement for Hedera.

I agree that's another approach and it can also help dApps to give the possibility to open any wallet. But both approaches are compatible this development doesn't force the dApps to use this extension pop way. I think at the point we are will be really helpful for them. Currently, we are in talks with some dApp developers and are aware of their difficulties.
Anyway, will be fantastic to also have the possibility you suggest. If you could create a new PR with examples of use and all the necessary code to support it, that would be great.

@zanctor zanctor mentioned this pull request Apr 5, 2024
2 tasks
@zanctor
Copy link

zanctor commented Apr 8, 2024

Hey, @franfernandez20. I've created a pull request with the approach we described earlier. Please, take a look.

@mgarbs mgarbs requested a review from hgraphql April 9, 2024 13:16
@franfernandez20
Copy link
Contributor Author

After translating the discussion to a forum with Wallet Connect core members, it was determined that the WalletConnectModal is not responsible for the popup extension. What they suggest is to use EIP-6963 standard to make the communication with an injected script. The suggested approach remains the same, with the added adaptation to EIP-6963.

image

@justynspooner
Copy link
Contributor

After translating the discussion to a forum with Wallet Connect core members, it was determined that the WalletConnectModal is not responsible for the popup extension. What they suggest is to use EIP-6963 standard to make the communication with an injected script. The suggested approach remains the same, with the added adaptation to EIP-6963.

image

wagmi has this capability built into its injected connector. Behind the scenes I believe it uses this library for Multi Injected Provider Discovery (eip-6963): https://github.com/wevm/mipd

It's worth checking out MIPD in a view to how we could use it for wallet browser extension discovery within Hedera WalletConnect.

@pwoosam
Copy link
Contributor

pwoosam commented Apr 17, 2024

Very cool feature! However, there are too many changes unrelated to the PR topic to provide a review of the changes and support merging this branch.

@franfernandez20
Copy link
Contributor Author

Very cool feature! However, there are too many changes unrelated to the PR topic to provide a review of the changes and support merging this branch.

I have updated with the latest integration and I am trying to keep the topic of the extension popup related only.

@franfernandez20 franfernandez20 requested review from a team as code owners May 27, 2024 14:00
@itsbrandondev
Copy link

hey everyone!

i wanted to share some thoughts regarding this pull request after reviewing it with @hgraphql and @franfernandez20. this hopefully provides some clarity for the group:

  • essentially this PR is an additional feature that allows the dApp to highlight specific wallets if they choose to do so in the modal. this is a similar UX for other ecosystems and could be helpful for dApps that require specialty wallets. it can also make the transition to WalletConnect easier for the ecosystem.
  • overall, the hedera wallet connect library remains focused on providing a single integration/button. this feature is a request from dApps and a common UX in other ecosystems.
  • also, dApps may opt to just utilize the single walletconnect button.
  • this PR is a little clearer for me now, and hopefully for others as well 👍

what objections remain for PR #82?

ps: i've attached a video that outlines it better

PR82.mp4

Copy link
Contributor

@hgraphql hgraphql left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the contribution!

Copy link
Contributor

@teacoat teacoat left a comment

Choose a reason for hiding this comment

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

I would suggest two improvements to this:

  • an example to test integrations against
  • a discover-extensions query to dynamically discover installed extensions and their id's rather than needing to manually enter the id of each wallet in "hedera-extension-query-"

I think the first one is a must-have for this to be production ready, but the second one can be added later I suppose, though it would be nice to have

@franfernandez20
Copy link
Contributor Author

I would suggest two improvements to this:

  • an example to test integrations against
  • a discover-extensions query to dynamically discover installed extensions and their id's rather than needing to manually enter the id of each wallet in "hedera-extension-query-"

I think the first one is a must-have for this to be production ready, but the second one can be added later I suppose, though it would be nice to have

Yeah @teacoat two good suggestions. There we have both in the last commits.

@franfernandez20
Copy link
Contributor Author

I attached a video to show a demo of the example app.

Grabacion.de.pantalla.2024-06-04.a.las.17.14.14.mov

Copy link
Contributor

@gregscullard gregscullard left a comment

Choose a reason for hiding this comment

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

LGTM

franfernandez20 and others added 4 commits June 5, 2024 19:27
Added a new parameter to the `DAppConnector` constructor that allows
defining the extension IDs that automatically pop up when pairing or
sending requests to the wallet.

This contribution includes additional functions that allow for checking
whether a specific extension is installed and responding. The available
extensions are listed under the `extensions` property of the
`DAppConnector` class. To establish a session that will automatically
activate the extension, use the `connectExtension` method instead of the
`connect` method.

Signed-off-by: Fran Fernandez <fran@kabila.app>
Signed-off-by: Fran Fernandez <fran@kabila.app>
Signed-off-by: Fran Fernandez <fran@kabila.app>
Signed-off-by: Fran Fernandez <fran@kabila.app>
@itsbrandondev itsbrandondev merged commit 70848f7 into hashgraph:main Jun 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants