airgap-it / beacon-sdk

The beacon sdk allows developers of dApps and wallets on Tezos to implement the wallet interaction standard tzip-10.
https://walletbeacon.io
MIT License
103 stars 65 forks source link

Attempting client dapp connection throws "is not a function" error #124

Closed fredcy closed 3 years ago

fredcy commented 3 years ago

Describe the bug (current behavior) Calling new DAppClient() results in a fatal error, reported to the Chrome console as

TypeError: sodium.crypto_generichash is not a function
    at crypto.ts:35
    at Generator.next (<anonymous>)
    at fulfilled (crypto.ts:2)

To Reproduce Steps to reproduce the behavior:

  1. Clone https://github.com/fredcy/beacon-test and checkout the 1.0 tag.
  2. Install and serve per the README
  3. View the page in the browser (per README)
  4. Open the JS console in Chrome
  5. See error

Expected (correct) behavior I expected a successful return value from the DAppClient() call.

Environment

AndreasGassmann commented 3 years ago

I will take a look ASAP.

fredcy commented 3 years ago

Thanks for the patch, @AndreasGassmann. It's working fairly well in my tests.

I still think that the way beacon-sdk is importing the libsodium module is not right. All the examples in the libsodium docs use require() but the beacon code uses import. I think that difference is why patching the code to add .default after the imported module name fixes things; but I admit to being baffled at these subtle differences between CommonJS modules and ES6 modules. I patched a local copy of beacon-sdk to import libsodium via require() rather than import and that version works OK -- it does not encounter the "is not a function" error. I never got the wallet choose popup but I may have to change how the qrcode module is imported as well.

fredcy commented 3 years ago

This may be a separate problem, but when I came back to my app today, still displaying in my browser from a working test yesterday, it did not work today: I would see a spinner in my page about connecting but then that would stop and nothing more would happen. Refreshing the page made no difference. Killing the tab with Kukai wallet did not help. What did work is to delete the localstorage for the page; then the wallet chooser comes up again.

AndreasGassmann commented 3 years ago

Glad to hear the fix worked as a temporary workaround. But I will definitely look into that and hopefully fix it in an upcoming version.

I would see a spinner in my page about connecting.

Are you talking about a request being sent, but then not appearing in the wallet? Or do you have some separate UI that shows up during connection? Because if I'm not mistaken, we don't show a spinner while connecting.

fredcy commented 3 years ago

I see an animated "Request sent" thing appear on the app page for about 2 seconds after the requestPermissions() call. Nothing happens in the wallet. No wallet chooser appears. The spinner disappears quickly. But I was able to get a screenshot after a few tries. It does this on every reload in the current state, and I don't get a return from the requestPermissions() promise. This "Request sent" UI gizmo is not something explicit in my app.

request_sent

AndreasGassmann commented 3 years ago

Yes, so this is the toast that is shown when a request is sent to the wallet. It will re-use the connection you have previously established (eg. P2P wallet or extension). To "reset" the connection, you have to do dappClient.setActiveAccount(), which then means the next permission request will trigger another "pairing" alert.

What wallet did you pair? If it was Kukai, you should see "increased network activity" to the matrix node. With increased network activity I mean that you should see a couple requests being made shortly after you do the permission request. This activity should also be happening on Kukai.

There was a bug regarding reconnects, but I thought that this was introduced in the 2.0.2 changes. Can you use the latest 2.0.2 beta and see if that works?

fredcy commented 3 years ago

Calling .setActiveAccount() did not work to reset things to where the wallet chooser comes up. But .removeAllAccounts() does.

RomarQ commented 3 years ago

I'm also getting the error sometimes. Resetting the storage is a workaround as already pointed above.

image

AndreasGassmann commented 3 years ago

@fredcy I just tested it again and I'm sure that await client.setActiveAccount() will set the state in a way that will trigger the pairing window again in the next request. If it is not the case for you, could you please share your code?

AndreasGassmann commented 3 years ago

@RomarQ This looks like a separate issue to me. Could you provide a way to reproduce this? I checked the code at the mentioned location (PostMessageClient.ts:89) and this code handles handshake responses from Browser Extensions. At what stage of the dApp flow does this happen? On page load or after pairing?

fredcy commented 3 years ago

I still think that the way beacon-sdk imports libsodium-wrappers is fragile -- in that it should perhaps use require rather than import -- but I've switched my app to build with the Vue framework and something about that context allows things to work. All the underlying babel and vue-client and webpack and such machinery is opaque and magic to me yet. That said, my original "is not a function" issue is moot now and I'm ok with closing this issue.

client.setActiveAccount() is working OK for me now. I've modified my app to do more explicit (user-visible) management of the active and known accounts, and it works OK. When I come back to an open app page after some hours I think I sometimes find that that although there is an active account it doesn't connect properly to the wallet, but that is hard to reproduce and test. So I think that too is OK for now.

Thanks for all your help with this.

AndreasGassmann commented 3 years ago

@fredcy I didn't look into the "build issues" yet. I am planning to do this after the 2.2.0 release.