fix: handling invalid session state #4139
Conversation
…ming keychain exists and removes session if deemed invalid
| await this.deleteSession(topic); | ||
| } else { | ||
| await this.deleteSession({ topic, emitEvent: false }); | ||
| } else if (this.client.core.pairing.pairings.keys.includes(topic)) { |
There was a problem hiding this comment.
Nice improvement 👍
In retrospect I don't recall why we didn't combine pairing and session handling into this top level disconnect/why it was session-only and pairings had to be handled explicitly via core.pairing.disconnect()
| @@ -446,7 +451,6 @@ export class Engine extends IEngine { | |||
| }; | |||
|
|
|||
| public getPendingSessionRequests: IEngine["getPendingSessionRequests"] = () => { | |||
| this.isInitialized(); | |||
There was a problem hiding this comment.
Por que no? Isn't pendingRequest a store that needs to be initialised during init?
There was a problem hiding this comment.
this isInitialized refers to engine's initialization and the cleanup is ran before initialized is set to true
| }); | ||
| this.client.proposal.getAll().forEach((proposal) => { | ||
| if (isExpired(proposal.expiry)) proposalIds.push(proposal.id); | ||
| }); | ||
| await Promise.all([ | ||
| ...sessionTopics.map((topic) => this.deleteSession(topic)), | ||
| ...sessionTopics.map((topic) => this.deleteSession({ topic })), |
There was a problem hiding this comment.
Do we want emitEvent to be true here? May be very noisy if the client cleans up a lot of sessions but not sure if suppressing emit would effectively be a breaking change to current behaviour.
There was a problem hiding this comment.
Actually it makes sense to emit, as expirer can delete sessions while the client is active, and the implementing party might not be subscribed to expirer events thus missing the delete
This seems to be fixed with this PR: WalletConnect/walletconnect-monorepo#4139 so that is why I force a newer WC version since web3-onboard doesn’t have a release that uses it.
* fix: wc no matching key: keychain This seems to be fixed with this PR: WalletConnect/walletconnect-monorepo#4139 so that is why I force a newer WC version since web3-onboard doesn’t have a release that uses it. * fix: wc no matching key. session This is an attempt to fix this WC error, but it doesn’t always work. Even more this error is reproducible only with 1inch. Steps to reproduce the error: 1. Open your safe and login into an account 2. Open 1inch and select connect with walletconnect 3. On your safe, paste the link in the wc pairing field, but don’t approve 4. Refresh the page 5. Get a new pairing link from 1inch and try to pair again 6. You get the error With the fix in this commit, we clean up on page unload this way the localstorage of walletconnect is left with an empty propsal and the next time the user tries to approve it should work. This is only a workaround. I think that this is a bug in walletconnect and they should better take care of their localstorage.
Description
Implemented several improvements in regards to handling invalid state such as having sessions without their corresponding keychain.
session_deleteto notify dapp that session got deletedType of change
How has this been tested?
tests
dogfooding
Checklist