Fix/ACTIVE_ACCOUNT_SET bugs

[klassare]

Fixes #757 & #758

The proper way to fix #757 would be to create a destroy() function that would make the Transport object eligible for garbage collection. That would reduce the risk for many bugs and prevent memory leakage. But since there is a concern for introducing regression bugs, I decided to keep the code changes to a minimum.

[IsaccoSordo]

Hi @klassare, thanks for contributing to Beacon. Unfortunately this PR introduces a regression. If you:

1. Successfully sync with a wallet through walletconnect
2. Disconnect the dApp
3. Sync again with a wallet always through walletconnect
4. Change the active account from the wallet side through a session_update

The last step won't be reflected on the dApp because all listeners have been destroyed and therefore the active account won't change (also an error gets displayed in the console)

[AlexandrosGounis]

@IsaccoSordo, I am unable to reproduce a regression given the steps you described. Could you please specify how you disconnect (on the DApp side instead of the wallet side) and did you use kukai with scanning/pasting QR codes to do that? I tried a clean build and I could not reproduce this issue using ghostnet.kukai.app, could you please share a quick video of your process?

[IsaccoSordo]

In the example dApp, if you check the logs, you will see an error saying "no request found for id," which is thrown by our handleResponse function. This indicates that it has not fully acknowledged an internal message (possible invalid state). Your PR does not address this issue; you may call it the “symptom” of the problem.

The real issue, which is under investigation, is why we end up with multiple subscriptions active at the same time. Forcefully resetting the listeners property to an empty array [] might seem to fix the issue (it may work in the example dApp; however, I was able to break it), but the underlying problem persists.

The complete solution should include:

1. As you correctly stated at the beginning of this PR, implementing a destroy function to handle cleanup.
2. Using a different data structure, such as a Map, to manage all listeners.

As seen in the addListener method within the Transport abstract class, we are adding new subscriptions to the listeners array without checking if the event is already being listened to.

TLDR: We need to update our subscription logic to avoid potential duplicates and memory leaks.

EDIT: I hope this message is much clearer than before.

[klassare]

I get the "no request found for id" error twice when I try to reproduce it against master. So doesn't seem like a regression to me. On the contrary...

[IsaccoSordo]

@klassare @AlexandrosGounis I apologise for the confusion, I have completely rewritten my previous message to explain better the issue.

[IsaccoSordo]

I'm closing this PR in favor of #764. However, I agree that we need to add a console.log when subscribing to ACTIVE_ACCOUNT_SET in the example dApp. This change is also included in #764.

[klassare]

Including #dc9f86f1e92176d2938df1d5f0639f15f8b9506b should be a no brainer. subscribeToSessionEvents is called twice when refreshState is executed. But I've already explained that in the #758 issue.

[IsaccoSordo]

Please when comparing code changes refer to our develop branch, as it holds our latest changes. With closeSignClient we now are removing all active subscriptions before subscribing with a new signClient instance. In any case that part of the code needs to be refactored to fully support broadcast channel for proxying requests between multiple tabs.

[klassare]

refreshState has the same bug in the develop branch.