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

🦊 Metamask integration POC #4835

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

🦊 Metamask integration POC #4835

wants to merge 7 commits into from

Conversation

thesan
Copy link
Member

@thesan thesan commented Apr 17, 2024

Demo

  • @chainsafe/polkadot-snap fails on FF for me so the demo only works on Chrome based browsers AFAIK.
  • The demo points at my publish version of the snap, so it only works with Metamask Flask.
  • The snap works with HTTP RPC endpoints only (adding WS support is a headache). So the Apps implementing this should start storing the HTTP RPC link too. In the meantime it works with a simple text replacement hack. Done ✔️

@thesan thesan requested a review from kdembler April 17, 2024 16:41
Copy link

vercel bot commented Apr 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
dao ✅ Ready (Inspect) Visit Preview Jun 4, 2024 6:48pm
pioneer-2 ✅ Ready (Inspect) Visit Preview Jun 4, 2024 6:48pm
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Jun 4, 2024 6:48pm

@thesan thesan marked this pull request as draft April 17, 2024 16:43
@thesan
Copy link
Member Author

thesan commented Apr 26, 2024

@kdembler the official snap seems to work now but it still reports an error when connecting. Also I it might have been working before the last release, but I was focussed on this error before realizing that the wsRpcUrl should actually be the http rpc url 😕.

I reported the issue (with the error message): ChainSafe/metamask-snap-polkadot#231

Going forward:

  • Some more QA is needed especially with the real (i.e not flask) Metamask and with extrinsics that are Joystream specific.
  • The apps supporting this should store the http rpc endpoints in addition to the ws one.
  • Also maybe this should be enabled on Chrome based browser only for now. I had forgotten about this and got confused on FF today 🤦

@thesan
Copy link
Member Author

thesan commented May 31, 2024

@kdembler I now get the 0.9.0 version of the snap with MetaMask:
image

However it still doesn't work on FF for me. I get to this screen but nothing happens:
image
Can you confirm it works for you on FF ?

@thesan thesan marked this pull request as ready for review May 31, 2024 11:18
Comment on lines +65 to +67
local: 'http://127.0.0.1:9933', // TODO: check
testnet: TESTNET_NODE_HTTP_RPC,
'local-mocks': 'http://127.0.0.1:9933', // TODO: check
Copy link
Member

Choose a reason for hiding this comment

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

Do we need those TODOs?

Copy link
Member Author

Choose a reason for hiding this comment

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

No I did check !

Comment on lines +26 to +38
const endpointReducer: Reducer<NetworkEndpoints | undefined, Optional<NetworkEndpoints, 'nodeHttpRpcEndpoint'>> = (
_,
newEndpoints
): NetworkEndpoints => ({
...newEndpoints,
nodeHttpRpcEndpoint:
newEndpoints.nodeHttpRpcEndpoint ??
newEndpoints.nodeRpcEndpoint
.replace('ws:', 'http:')
.replace('wss:', 'https:')
.replace('ws-rpc', 'http-rpc')
.replace(':9944', ''),
})
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this logic after we added HTTP endpoint to config explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually no but we should make sure that the HTTP endpoints are defined in the environment.
Also the code should get the endpoint from the playground configs (I assumed it wasn't in there so I left the logic but it is in the configs e.g: https://atlas-next.joystream.org/network/config.json).

@kdembler
Copy link
Member

Walletconnect seems to not work for me after this change. When I select the option, I don't get the connect popup

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.

None yet

2 participants