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: implement metamask starknet snap #206

Conversation

stanleyyconsensys
Copy link

@stanleyyconsensys stanleyyconsensys commented Nov 23, 2023

Description:
the PR is to add metamask starknet snap as a wallet options on the get-starknet UI and return a compilable wallet object to get-starknet as well as the other wallets

the concept is same with the other PR: #195

Concept:
Due to MetaMask limitation, the SNAP itself can not directly inject a object into window instance, and snap itself is communicated via JSONRPC, SNAP can only return JSON-serializable response
https://docs.metamask.io/snaps/reference/rpc-api/

Hence we create a wallet object in get-starknet codebase, the object itself will setup as a bridge to communicate with SNAP via JSONRPC
We then runtime inject this object into get-starknet
image

Detail implementation

  • create a class object named MetaMaskSnap to communicate with the snap via the window ethereum provider
  • create a class object named MetaMaskAccount to extend the Account class from starknet package, and overriding execute, signMessage, declare with MetaMaskSnap
  • create a class object named MetaMaskSigner to implement the SignerInterface interface from starknet package, and implement all member methods with MetaMaskSnap
  • create a class object named MetaMaskSnapWallet to implement the same API as IStarknetWindowObject
    • when enable method trigger, it will request user to install starknet snap into metamask if not installed
  • inject MetaMaskSnapWallet when getAvailableWallets, getPreAuthorizedWallets, getDiscoveryWallets, getLastConnectedWallet method are consuming
  • enhance example page to demo the integration

@naorye2
Copy link
Contributor

naorye2 commented Dec 5, 2023

@stanleyyconsensys
It seems that you add the snap implementation into get-starknet code.
starknet_metamask should be injected from the snap itself.

@stanleyyconsensys
Copy link
Author

stanleyyconsensys commented Dec 6, 2023

hould be injected from the snap itself.

hi @naorye2 thanks for the review
there is no way to injected from the snap like other wallet
such as
return a account object or injected the account object into window object

as snap is a JS script that run on metamask, it act like a serverless API, hence, the script itself cant inject anything
Plus
snap can be only communicate via rpc, means the snap will not able to return any non JSON-serializable response (such as wallet object)

@naorye2
Copy link
Contributor

naorye2 commented Dec 6, 2023

@stanleyyconsensys got it. I suggest the following:

  1. Your wallet implementation must be hosted on your behalf so you eventually will provide a url for a script.
  2. get-starknet will hold a reference to that script (under discovery.js).
  3. get-starket will check whether metamask loaded with snaps support (the code that you added under MetaMaskSnap.GetProvider) and if it is, it will load the script of yours.
  4. From that point, starknet_metamask will be injected by your script.

WDYT?

@stanleyyconsensys
Copy link
Author

account object or injected the

@naorye2

thank you for the advice
it is a good idea
but may i ask how about get-starknet adding a npm package from us, in that way, user does not required to load our jS via internet on the fly, instead, it compile together with get-starknet

the major different is, when we using async load script, there may be some issue from user ENV, may blocking the access of the script

with npm package, get-starknet would just need to maintain the version of our package, but it may also create a dependency on us

how does it sound like to you?

@naorye2
Copy link
Contributor

naorye2 commented Dec 7, 2023

@stanleyyconsensys
This is sounds like a better solution :)
However, this package will be bundled along the build process into a separate file that will be imported dynamically according to metamask existence and snap support.
The check will be occurred right on the package startup.

@stanleyyconsensys
Copy link
Author

@stanleyyconsensys This is sounds like a better solution :) However, this package will be bundled along the build process into a separate file that will be imported dynamically according to metamask existence and snap support. The check will be occurred right on the package startup.

thank you for the feedback
I think we can discuss all together after Alex has sometime

@naorye2
Copy link
Contributor

naorye2 commented Dec 13, 2023

@stanleyyconsensys
I am working on a PR for metamask starknet brisge as we spoke.
Currently I am using the logic you wrote but than I realized that this logic is missing async loading of metamask (in case metamask loads minute after get-starkent, we will miss it).
I saw that approach: https://github.com/MetaMask/detect-provider
Is it good enough for metamask detection (than we will use provider && hasSnapSupport(provider))?

import detectEthereumProvider from '@metamask/detect-provider';

async function hasSnapSupport(provider: any) {
  try {
    await provider.request({
      method: "wallet_getSnaps",
    })
    return true
  } catch {
    return false
  }
}

async function bootstrapMetamaskBridge() {
  if (window.hasOwnProperty("starknet_metamask")) {
    return;
  }

  const provider = await detectEthereumProvider()
  if (!provider) {
    return;
  }

  const snapSupport = await hasSnapSupport(provider)
  if (!snapSupport) {
    return;
  }

  const MetaMaskSnapWallet = await import('starknet-metamask')
  window["starknet_metamask"] = new MetaMaskSnapWallet(provider)
}

export {bootstrapMetamaskBridge}

@naorye2
Copy link
Contributor

naorye2 commented Dec 13, 2023

@stanleyyconsensys
#208

@stanleyyconsensys
Copy link
Author

@naorye2 , the detectEthereumProvider can be useful if the client browser only install metamask,

but in some edge case, if the user has installed some wallets which overrided the Ethereum object, it will lead to using the wrong provider

i would suggest you can combine both way to inject the provider, may i ask where will you inject the bootstrapMetamaskBridge?

@naorye2
Copy link
Contributor

naorye2 commented Dec 14, 2023

@stanleyyconsensys

According to @metamask/detect-provider, it verifies whether it is metamask.
image

Can you review my implementation?

bootstrapMetamaskBridge is called in packages/core/src/main.ts in getStarknet() function.

@stanleyyconsensys
Copy link
Author

i think you can try that way first, and i will test it by enabling Phantom and coinbase wallet later

@stanleyyconsensys
Copy link
Author

stanleyyconsensys commented Dec 15, 2023

@naorye2 here is the consensys package for supporting get-starknet,
https://www.npmjs.com/package/@consensys/get-starknet

for your information, the version of that module doesn't needed to corresponding with the snap version

Hence, when a snap version update, it is not always needed to update the version of the module

therefore, we have add a snapVersion parameter when constructing the MetaMaskSnapWallet for flexible changes

the current beta version for the snap will be '2.3.0-staging'

it is a dev version
we will release a stable version once the test is success

@naorye2
Copy link
Contributor

naorye2 commented Dec 19, 2023

@stanleyyconsensys
I integrated your package (dev version for the meanwhile).
Can you expose MetaMaskProvider?

Please review my pr #208.

@stanleyyconsensys
Copy link
Author

Sure, let me export all the type and component for you

@naorye2
Copy link
Contributor

naorye2 commented Mar 17, 2024

Handled here #208

@naorye2 naorye2 closed this Mar 17, 2024
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.

4 participants