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

Don't use getter for accessing walletconnector in close() method to prevent race condition inside getter when session is already killed #208

Merged
merged 5 commits into from
Dec 18, 2019

Conversation

mmv08
Copy link
Contributor

@mmv08 mmv08 commented Dec 10, 2019

The bug:
If you use walletconnect to connect your wallet and then close session from the mobile wallet while currently being inside the dapp, it will show you an invalid QRCode modal.

Steps to reproduce:

  1. Connect with walletconnect on https://web3connect.com/
  2. Close session in the mobile wallet
  3. QRCode modal will open, scanning it would do nothing

The reason for this bug
When user disconnects the session, _handleDisconnectSession method is called inside @walletconnect/core and sets property connected on walletConnector to false. Then whatever uses the core (let's say web3connect), receives the disconnect event and wants to perform a cleanup and close walletconnect connection (not actually sure whether this is the best way to handle this, but this is how it's done in the web3connect example app: https://github.com/web3connect/web3connect/blob/master/example/src/App.tsx#L346). It calls .close() and close tries to get the walletconnector via getWalletConnector, inside this method it checks if wc.connected is true and if not, it tries to initialize a new session and shows QRCode modal.

Background info
I also thought about handling it inside subscribeWalletConnector:

wc.on('disconnect', (error, payload) => {
but this fix seemed easier and also possibility of the lib trying to start a new session inside .close method felt wrong. If I am missing something, please let me know

@mmv08 mmv08 changed the title Don't use getter for accessing walletconnector in close() method to prevent race condition inside getter Don't use getter for accessing walletconnector in close() method to prevent race condition inside getter when session is already killed Dec 10, 2019
// killed (wc.connected is false), it would try to initialize a new session in the getter
// with an invalid wc.uri

const wc = this.wc
Copy link
Contributor

@pedrouid pedrouid Dec 10, 2019

Choose a reason for hiding this comment

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

We should check if wc exists and is connected. Perhaps this would be best if it were changed as follows:

async close() {
  let wc
  if (this.wc && this.wc.connected) {
    wc = await this.getWalletConnector()
  } else if (this.wc) {
    wc = this.wc
  }

  if (wc) {
    await wc.killSession()
  }
  await this.stop()
  this.emit('close', 1000, 'Connection closed')
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we could pass an optional argument to getWalletConnector to disable the re-connection in case it's not connected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedrouid I went with second solution because I thought that an ability to explicitly disable this behaviour for the getter would be better.

@pedrouid pedrouid merged commit aabdc60 into WalletConnect:master Dec 18, 2019
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.

2 participants