From 5b0fa776bdbb83c8ed3d806017a17ddbcbad6585 Mon Sep 17 00:00:00 2001 From: Gancho Radkov Date: Thu, 21 Dec 2023 10:37:59 +0200 Subject: [PATCH 1/5] fix: captures `respond` rejection when session is disconnect & remove the pending request --- packages/sign-client/src/controllers/engine.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/sign-client/src/controllers/engine.ts b/packages/sign-client/src/controllers/engine.ts index 7d1feb5b7..b39d7a28a 100644 --- a/packages/sign-client/src/controllers/engine.ts +++ b/packages/sign-client/src/controllers/engine.ts @@ -381,6 +381,13 @@ export class Engine extends IEngine { public respond: IEngine["respond"] = async (params) => { await this.isInitialized(); + + // if the session is already disconnected, we can't respond to the request so we need to delete it + await this.isValidSessionTopic(params.topic).catch((error) => { + this.cleanupAfterResponse(params); + throw error; + }); + await this.isValidRespond(params); const { topic, response } = params; const { id } = response; From 3bba512cc8764b11be7b80b8274a1df9b55395ba Mon Sep 17 00:00:00 2001 From: Gancho Radkov Date: Thu, 21 Dec 2023 10:38:33 +0200 Subject: [PATCH 2/5] feat: tests --- packages/sign-client/test/sdk/client.spec.ts | 84 +++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/packages/sign-client/test/sdk/client.spec.ts b/packages/sign-client/test/sdk/client.spec.ts index 44726bcb8..323f7b7de 100644 --- a/packages/sign-client/test/sdk/client.spec.ts +++ b/packages/sign-client/test/sdk/client.spec.ts @@ -6,7 +6,7 @@ import { } from "@walletconnect/jsonrpc-utils"; import { RelayerTypes } from "@walletconnect/types"; import { getSdkError, parseUri } from "@walletconnect/utils"; -import { expect, describe, it, vi, beforeEach, afterEach } from "vitest"; +import { expect, describe, it, vi } from "vitest"; import SignClient, { WALLETCONNECT_DEEPLINK_CHOICE } from "../../src"; import { @@ -346,6 +346,88 @@ describe("Sign Client Integration", () => { await throttle(1000); await deleteClients(clients); }); + /** + * this test simulates the case where a session is disconnected + * while session request is being approved + * the queue should continue operating normally after the `respond` rejection + */ + it("should handle session disconnect durign request approval", async () => { + // create the clients and pair them + const { + clients, + sessionA: { topic: topicA }, + } = await initTwoPairedClients({}, {}, { logger: "error" }); + const dapp = clients.A as SignClient; + const wallet = clients.B as SignClient; + const { uri, approval } = await dapp.connect({ + requiredNamespaces: {}, + }); + + let topicB = ""; + await Promise.all([ + new Promise((resolve) => { + wallet.once("session_proposal", async (args) => { + const { id } = args.params; + await wallet.approve({ + id, + namespaces: TEST_NAMESPACES, + }); + resolve(); + }); + }), + wallet.pair({ uri: uri! }), + new Promise(async (resolve) => { + const session = await approval(); + topicB = session.topic; + resolve(); + }), + ]); + + const expectedRequests = 5; + let receivedRequests = 0; + await Promise.all([ + new Promise((resolve) => { + clients.B.on("session_request", async (args) => { + receivedRequests++; + const { id, topic } = args; + + // capture the request on topicB, disconnect and try to approve the request + if (topic === topicB) { + await new Promise(async (_resolve) => { + await wallet.disconnect({ + topic, + reason: getSdkError("USER_DISCONNECTED"), + }); + _resolve(); + }); + } + await clients.B.respond({ + topic, + response: formatJsonRpcResult(id, "ok"), + }).catch((err) => { + // eslint-disable-next-line no-console + console.log("respond error", err); + }); + if (receivedRequests >= expectedRequests) resolve(); + }); + }), + new Promise((resolve) => { + clients.A.request({ + topic: topicB, + ...TEST_REQUEST_PARAMS, + }); + resolve(); + }), + Array.from(Array(expectedRequests).keys()).map(() => + clients.A.request({ + topic: topicA, + ...TEST_REQUEST_PARAMS, + }), + ), + ]); + await throttle(1000); + await deleteClients(clients); + }); }); }); }); From 07317555a02b1a8c96ac5c535d300ab932e2cdff Mon Sep 17 00:00:00 2001 From: Gancho Radkov Date: Thu, 21 Dec 2023 10:50:22 +0200 Subject: [PATCH 3/5] refactor: removes any pending requests when session is disconnected --- packages/sign-client/src/controllers/engine.ts | 5 +++++ packages/sign-client/test/sdk/client.spec.ts | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/sign-client/src/controllers/engine.ts b/packages/sign-client/src/controllers/engine.ts index b39d7a28a..0fb571764 100644 --- a/packages/sign-client/src/controllers/engine.ts +++ b/packages/sign-client/src/controllers/engine.ts @@ -497,6 +497,11 @@ export class Engine extends IEngine { this.client.core.storage .removeItem(WALLETCONNECT_DEEPLINK_CHOICE) .catch((e) => this.client.logger.warn(e)); + this.getPendingSessionRequests().forEach((r) => { + if (r.topic === topic) { + this.deletePendingSessionRequest(r.id, getSdkError("USER_DISCONNECTED")); + } + }); }; private deleteProposal: EnginePrivate["deleteProposal"] = async (id, expirerHasDeleted) => { diff --git a/packages/sign-client/test/sdk/client.spec.ts b/packages/sign-client/test/sdk/client.spec.ts index 323f7b7de..caba7a7e5 100644 --- a/packages/sign-client/test/sdk/client.spec.ts +++ b/packages/sign-client/test/sdk/client.spec.ts @@ -351,7 +351,7 @@ describe("Sign Client Integration", () => { * while session request is being approved * the queue should continue operating normally after the `respond` rejection */ - it("should handle session disconnect durign request approval", async () => { + it("continue processing requests queue after respond rejection due to disconnected session", async () => { // create the clients and pair them const { clients, From 8f7b6109d839996db8baf573eda30678da99b854 Mon Sep 17 00:00:00 2001 From: Gancho Radkov Date: Thu, 21 Dec 2023 10:54:24 +0200 Subject: [PATCH 4/5] refactor: moves cleanup in `isValidRespond` --- packages/sign-client/src/controllers/engine.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/sign-client/src/controllers/engine.ts b/packages/sign-client/src/controllers/engine.ts index 0fb571764..b25b198dd 100644 --- a/packages/sign-client/src/controllers/engine.ts +++ b/packages/sign-client/src/controllers/engine.ts @@ -381,13 +381,6 @@ export class Engine extends IEngine { public respond: IEngine["respond"] = async (params) => { await this.isInitialized(); - - // if the session is already disconnected, we can't respond to the request so we need to delete it - await this.isValidSessionTopic(params.topic).catch((error) => { - this.cleanupAfterResponse(params); - throw error; - }); - await this.isValidRespond(params); const { topic, response } = params; const { id } = response; @@ -1051,7 +1044,7 @@ export class Engine extends IEngine { }; private cleanupAfterResponse = (params: EngineTypes.RespondParams) => { - this.deletePendingSessionRequest(params.response.id, { message: "fulfilled", code: 0 }); + this.deletePendingSessionRequest(params.response?.id, { message: "fulfilled", code: 0 }); // intentionally delay the emitting of the next pending request a bit setTimeout(() => { this.sessionRequestQueue.state = ENGINE_QUEUE_STATES.idle; @@ -1400,7 +1393,11 @@ export class Engine extends IEngine { throw new Error(message); } const { topic, response } = params; - await this.isValidSessionTopic(topic); + // if the session is already disconnected, we can't respond to the request so we need to delete it + await this.isValidSessionTopic(topic).catch((error) => { + this.cleanupAfterResponse(params); + throw error; + }); if (!isValidResponse(response)) { const { message } = getInternalError( "MISSING_OR_INVALID", From 71ae21807d140cfe06cd425f6e2568a67dcf03ec Mon Sep 17 00:00:00 2001 From: Gancho Radkov Date: Thu, 21 Dec 2023 13:23:42 +0200 Subject: [PATCH 5/5] refactor: uses try/catch to handle async and explicitly checks if id is provided before trying to cleanup pending request --- packages/sign-client/src/controllers/engine.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/sign-client/src/controllers/engine.ts b/packages/sign-client/src/controllers/engine.ts index b25b198dd..970f9928d 100644 --- a/packages/sign-client/src/controllers/engine.ts +++ b/packages/sign-client/src/controllers/engine.ts @@ -1044,7 +1044,7 @@ export class Engine extends IEngine { }; private cleanupAfterResponse = (params: EngineTypes.RespondParams) => { - this.deletePendingSessionRequest(params.response?.id, { message: "fulfilled", code: 0 }); + this.deletePendingSessionRequest(params.response.id, { message: "fulfilled", code: 0 }); // intentionally delay the emitting of the next pending request a bit setTimeout(() => { this.sessionRequestQueue.state = ENGINE_QUEUE_STATES.idle; @@ -1393,11 +1393,13 @@ export class Engine extends IEngine { throw new Error(message); } const { topic, response } = params; - // if the session is already disconnected, we can't respond to the request so we need to delete it - await this.isValidSessionTopic(topic).catch((error) => { - this.cleanupAfterResponse(params); + try { + // if the session is already disconnected, we can't respond to the request so we need to delete it + await this.isValidSessionTopic(topic); + } catch (error) { + if (params?.response?.id) this.cleanupAfterResponse(params); throw error; - }); + } if (!isValidResponse(response)) { const { message } = getInternalError( "MISSING_OR_INVALID",