search

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

Destructor not working properly #635

Closed klassare closed 1 year ago

klassare commented 1 year ago

Describe the bug (current behavior) In some situations client.destroy() has an inconvenient delay. This can cause issues for logic that depends on the client first being destroyed. For example like with the "reset & refresh" button in the example dapp.

To Reproduce Steps to reproduce the behavior:

  1. Open the example dapp and "Reset & refresh".
  2. Pair with Kukai and approve the permission request
  3. Send an operation request

Expected (correct) behavior destroy() should always resolve within a few seconds

Environment

Additional context Branch: feat/transport-kukai-wc2

AndreasGassmann commented 1 year ago

We investigated this issue and those are the findings:

  1. We were not able to reproduce the delay with the destroy method. In the past, I feel like the button did at times not properly reset & refresh the page (potentially the delay mentioned here), but after all of the other fixes have been merged, I could no longer reproduce it. I also made a change to the UI to disable the button after clicking it, so we have better visual feedback.
  2. The destroy method does a couple of things to clean up. Besides removing local storage entries, it also disconnects the transport and waits for this action to be done. The problem is that we can't really control what happens internally, eg. inside the signClient. When we call await signClient.core.pairing.disconnect({ topic: pairing.topic }), if this hangs for some reason, then the await client.destroy() will also hang. If we don't await everything internally, then we run into the risk that not everything is cleaned up and "ready" when the destroy resolves.

Unless we can reliably reproduce the error and pinpoint exactly what is causing the delay, I think we have to keep it as it is, unless anyone has another suggestion.

BTW: The PR linked in this issue does not directly address this issue, because of the points mentioned above. However, it does address another issue where the destroy method could resolve while not everything was cleaned up internally.

IsaccoSordo commented 1 year ago

Hi @klassare, the issue was caused by some promises that weren't resolving "on time". With #639 we now await the termination of each instruction. This fix however will be available in the next build. Relevant lines of code: Check PR #639

klassare commented 1 year ago

Looks like #639 fixes it