fix: clean up dangling event listeners after auditing event listeners #81
Conversation
| reject(error); | ||
| }) | ||
| .finally(() => { | ||
| this.dappClient.off('message', dappClientMessageHandler); |
There was a problem hiding this comment.
we might only want this in the catch. If it's in the finally, we might unsubscribe before the event listener has processed a wallet_createSession response?.. @adonesky1
| if (initialConnectionMessageHandler) { | ||
| this.dappClient.off( | ||
| 'message', | ||
| initialConnectionMessageHandler, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think when @adonesky1 and I looked at this, we may have discovered that this listener was the only one also listening for wallet_sessionChanged, caching the value and/or notifying callbacks for it.
We need to verify subsequent wallet_sessionChanged events still come through
There was a problem hiding this comment.
i don't think handleMessage is written correctly to be a stand in here
There was a problem hiding this comment.
I can remove this unsubscription from here then if wallet_sessionChanged depends on it, but we run into an issue where this handler never removed, meaning if connect() is called multiple times, multiple handlers accumulate (each call adds a new one).
also seems like handleMessage is processing the same wallet_sessionChanged notifications, but that method is such a confusion pile I'm not even sure what to say regarding this.
There was a problem hiding this comment.
5217c82 moved removal of listener if payload has error, but might be open to issue I stated above
| if (initialConnectionMessageHandler) { | ||
| this.dappClient.off('message', initialConnectionMessageHandler); | ||
| initialConnectionMessageHandler = undefined; | ||
| } |
There was a problem hiding this comment.
i don't think we want to do this in the finally since the wallet_sessionChanged may not have arrived at this point
Explanation
This PR proposes a clean up to a couple dangling event listeners we had across
connect-multichain,connect-evmand mwp transport.1. ✅ connect-evm/src/connect.ts - Fixed duplicate
onNotificationlistener2. ✅ MultichainCore deeplinkConnect - Fixed potential listener leak
3. ✅ MWPTransport - Fixed duplicate
dappClient.on('message')listenerReferences
Checklist