fix: multichain sdk state + bug fixing#1358
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1358 +/- ##
=======================================
Coverage 74.99% 74.99%
=======================================
Files 184 184
Lines 4519 4519
Branches 1108 1108
=======================================
Hits 3389 3389
Misses 1130 1130 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .then(() => { | ||
| this.options.ui.factory.unload(); | ||
| this.options.ui.factory.modal?.unmount(); | ||
| this.state = 'connected'; |
There was a problem hiding this comment.
I see these referenced in a lot of places, would definitely make our lives easier if these 'connected', 'disconnected', etc.. states are an ENUM instead of hardcoded strings.
There was a problem hiding this comment.
this.state is typed and has autocomplete, there's no need for an enum here, the problem that enums have in my opinion to enforce strings is that you are forced then to pass the enum value, while in reality its just a string.
those values are fully typed and when ur typing the value it shows u which values are available, easy auto-complete
There was a problem hiding this comment.
my preference would be to use an enum. I mean the type right below SDKState is an enum too https://github.com/MetaMask/metamask-sdk/blob/main/packages/sdk-multichain/src/domain/multichain/index.ts#L10
There was a problem hiding this comment.
Can we threat this as a separate issue? gonna add it in notion
| const currentScopes = Object.keys(walletSession?.sessionScopes ?? {}) as Scope[]; | ||
| const proposedScopes = options?.scopes ?? []; | ||
| const isSameScopes = currentScopes.every((scope) => proposedScopes.includes(scope)) && proposedScopes.every((scope) => currentScopes.includes(scope)); | ||
| logger('isSameScopes', isSameScopes); |
There was a problem hiding this comment.
I'm imagining this log was for debugging purposes, do we want to keep it ?
Also, could we leave a // TODO comment here to come back to this implementation of isSameScopes, since we are only checking surface level scope for now, and not the actual accounts inside, as we previously discussed in one of our team syncs.
There was a problem hiding this comment.
logger only enables if debug options are active. In order to show the debugging logs u need to add debug to your storage and also define the scope, multichain-sdk:* or multichain-sdk:core or multichain-sdk:transports but they are not activated by default and not visible by default
There was a problem hiding this comment.
Around checking the accounts also, instead of adding the comment i propose to create ticket in notion
| } | ||
|
|
||
| private onWindowFocus(): void { | ||
| logger('onWindowFocus', this.isConnected()); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| } | ||
|
|
||
| private notifyCallbacks(data: unknown): void { | ||
| logger('notifyCallbacks', data); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| } | ||
|
|
||
| private handleMessage(message: unknown): void { | ||
| logger('handleMessage', message); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| const currentScopes = Object.keys(walletSession?.sessionScopes ?? {}) as Scope[]; | ||
| const proposedScopes = options?.scopes ?? []; | ||
| const isSameScopes = currentScopes.every((scope) => proposedScopes.includes(scope)) && proposedScopes.every((scope) => currentScopes.includes(scope)); | ||
| logger('onResumeSuccess isSameScopes', isSameScopes, currentScopes, proposedScopes); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| params: walletSession, | ||
| }); | ||
| resumeResolve(); | ||
| logger('onResumeSuccess finished'); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| logger('resuming session...'); | ||
| connection = new Promise<void>((resumeResolve, resumeReject) => { | ||
| if (this.dappClient.state === 'CONNECTED') { | ||
| logger('already connected, onResumeSuccess'); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| * Registers a callback for notifications from the wallet | ||
| */ | ||
| onNotification(callback: (data: unknown) => void): () => void { | ||
| logger('onNotification registered'); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
| logger('onNotification registered'); | ||
| this.notificationCallbacks.add(callback); | ||
| return () => { | ||
| logger('onNotification unregistered'); |
There was a problem hiding this comment.
same here, is this just a debug log or do we want to keep it ?
b961bb0 to
fb2d0c4
Compare
fb2d0c4 to
8a21cee
Compare
|
4 participants
Explanation
Main objetive of this PR is to ensure we are setting the right state for the multichain SDK with all transports and flows, we should be able to capture the current state, "loaded", "pending", "connecting", "connected" and "disconnected"
Together with that simple fix, we are also including some secondary issues that have also been addressed as part of this PR:
MWP transport not sending the right payload on RPC callbacks (Notifications) causing some glitches in the user interface (multichain react playground)
Added better logging to both, default and mwp transport.
Fix wrong state being reported, exclusively on Browser + Extension and default transport (show install modal flow)
Prevent default transport from timing out requests, happens because we were not defining the default transport and it considers they are expired already if timeout is 0 or undefined
If there's an error establishing connection with an existing stored transport, automatically remove it to allow the user to establish connection again on desired transport instead or leaving the Dapp in a connecting state permanently
References
Checklist
Note
Unifies connection state management across flows, fixes MWP notifications and session handling, adds default transport timeouts, introduces a transport logger namespace, and updates tests accordingly.
handleConnectionto setconnecting/connected/disconnectedconsistently acrossconnect,showInstallModal, and deeplink flows.pending; on stored transport setup, ensure final state isconnectedwhen applicable.multichain/transports/mwp):wallet_sessionChanged) for session-related RPCs and notify listeners before resolving.wallet_getSession/wallet_createSessionerrors; create session when missing; log active session withmetamask-sdk:transport.multichain/transports/default):metamask-sdk:transportnamespace to logger.disconnectedafter failed connections and refine state assertions across scenarios.Written by Cursor Bugbot for commit 8a21cee. This will update automatically on new commits. Configure here.