Skip to content

improve mwp connection management #84

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.

Sign up for GitHub

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

Open
adonesky1 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

improve mwp connection management #84

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
962628e
improve mwp connection management
adonesky1 Dec 12, 2025
5aab5d9
Merge branch 'main' into ad/improve-dapp-client-connection-handling
jiexi Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 84 additions & 10 deletions packages/connect-multichain/src/multichain/transports/mwp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,16 @@ export class MWPTransport implements ExtendedTransport {

private windowFocusHandler: (() => void) | undefined;

/**
* Tracks the current connection promise to prevent concurrent connection attempts
*/
private connectionPromise: Promise<void> | undefined;

/**
* Cleanup function for the message handler registered during fresh connections
*/
private connectionMessageHandler: ((message: unknown) => void) | undefined;

get pendingRequests() {
return this.__pendingRequests;
}
Expand Down Expand Up @@ -319,6 +329,12 @@ export class MWPTransport implements ExtendedTransport {
scopes: Scope[];
caipAccountIds: CaipAccountId[];
}): Promise<void> {
// Guard against concurrent connection attempts - return existing promise if one is in progress
if (this.connectionPromise) {
logger('connection already in progress, waiting for existing promise');
return this.connectionPromise;
}

Copy link
Contributor

@jiexi jiexi Dec 15, 2025

Choose a reason for hiding this comment

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

The connection promise will eventually timeout, so this won't get stuck forever, but I thought one of our goals was to avoid having to wait for that to occur before being allowed to attempt to establish a new connection

Copy link
Contributor

@jiexi jiexi Dec 15, 2025

Choose a reason for hiding this comment

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

i suppose we have delegated this to the disconnect() and connect() in the layer above this

const { dappClient } = this;

const session = await this.getActiveSession();
Expand All @@ -327,17 +343,29 @@ export class MWPTransport implements ExtendedTransport {
}

let timeout: NodeJS.Timeout;
const connectionPromise = new Promise<void>((resolve, reject) => {
this.connectionPromise = new Promise<void>((resolve, reject) => {
let connection: Promise<void>;
if (session) {
connection = new Promise<void>((resumeResolve, resumeReject) => {
if (this.dappClient.state === 'CONNECTED') {
this.onResumeSuccess(resumeResolve, resumeReject, options);
// Await onResumeSuccess to ensure proper error handling
this.onResumeSuccess(resumeResolve, resumeReject, options).catch(
resumeReject,
);
} else {
this.dappClient.once('connected', async () => {
this.onResumeSuccess(resumeResolve, resumeReject, options);
// Set up the connected event handler before calling resume
const onConnected = (): void => {
this.onResumeSuccess(resumeResolve, resumeReject, options).catch(
resumeReject,
);
};
this.dappClient.once('connected', onConnected);

// Handle resume failure - if resume throws, clean up and reject
dappClient.resume(session?.id ?? '').catch((err: Error) => {
this.dappClient.off('connected', onConnected);
resumeReject(err);
});
dappClient.resume(session?.id ?? '');
}
});
} else {
Expand All @@ -357,8 +385,16 @@ export class MWPTransport implements ExtendedTransport {
params: sessionRequest,
};

// just used for initial connection
this.dappClient.on('message', async (message: unknown) => {
// Clean up any existing message handler from a previous connection attempt
if (this.connectionMessageHandler) {
this.dappClient.off('message', this.connectionMessageHandler);
this.connectionMessageHandler = undefined;
}

// Create and track the message handler for cleanup
this.connectionMessageHandler = async (
message: unknown,
): Promise<void> => {
if (typeof message === 'object' && message !== null) {
if ('data' in message) {
const messagePayload = message.data as Record<
Expand All @@ -369,6 +405,15 @@ export class MWPTransport implements ExtendedTransport {
messagePayload.method === 'wallet_createSession' ||
messagePayload.method === 'wallet_sessionChanged'
) {
// Clean up this handler once we get the response
if (this.connectionMessageHandler) {
this.dappClient.off(
'message',
this.connectionMessageHandler,
);
this.connectionMessageHandler = undefined;
}

Comment on lines +408 to +416
Copy link
Contributor

@jiexi jiexi Dec 15, 2025

Choose a reason for hiding this comment

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

can we move all cleanup to the very last finally on line 468?

Copy link
Contributor

@jiexi jiexi Dec 15, 2025

Choose a reason for hiding this comment

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

i think this also needs to go into the disconnect()

if (messagePayload.error) {
return rejectConnection(messagePayload.error);
}
Expand All @@ -381,7 +426,9 @@ export class MWPTransport implements ExtendedTransport {
}
}
}
});
};

this.dappClient.on('message', this.connectionMessageHandler);

dappClient
.connect({
Expand All @@ -391,22 +438,39 @@ export class MWPTransport implements ExtendedTransport {
data: request,
},
})
.catch(rejectConnection);
.catch((err: Error) => {
// Clean up handler on connection failure
if (this.connectionMessageHandler) {
Copy link
Contributor

@jiexi jiexi Dec 15, 2025

Choose a reason for hiding this comment

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

same about moving this into the very last finally

this.dappClient.off(
'message',
this.connectionMessageHandler,
);
this.connectionMessageHandler = undefined;
}
rejectConnection(err);
});
},
);
}

timeout = setTimeout(() => {
// Clean up message handler on timeout
if (this.connectionMessageHandler) {
this.dappClient.off('message', this.connectionMessageHandler);
this.connectionMessageHandler = undefined;
}
reject(new TransportTimeoutError());
}, this.options.connectionTimeout);

connection.then(resolve).catch(reject);
});

return connectionPromise.finally(() => {
return this.connectionPromise.finally(() => {
if (timeout) {
clearTimeout(timeout);
}
// Clear the connection promise so future connect() calls can proceed
this.connectionPromise = undefined;
});
}

Expand All @@ -425,6 +489,16 @@ export class MWPTransport implements ExtendedTransport {
window.removeEventListener('focus', this.windowFocusHandler);
this.windowFocusHandler = undefined;
}

// Clean up any pending connection message handler
if (this.connectionMessageHandler) {
this.dappClient.off('message', this.connectionMessageHandler);
this.connectionMessageHandler = undefined;
}

// Clear the connection promise to allow fresh connection attempts
this.connectionPromise = undefined;

this.kvstore.delete(SESSION_STORE_KEY);
this.kvstore.delete(ACCOUNTS_STORE_KEY);
this.kvstore.delete(CHAIN_STORE_KEY);
Expand Down
Loading