-
Notifications
You must be signed in to change notification settings - Fork 252
Migrate MetaMask Connector to use new SDK (MMConnect) #2326
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
base: master
Are you sure you want to change the base?
Migrate MetaMask Connector to use new SDK (MMConnect) #2326
Conversation
|
@adonesky1 is attempting to deploy a commit to the Consensys Team on Vercel. A member of the Team first needs to authorize it. |
| this.metamaskInstance = await this.metamaskPromise; | ||
| // TODO: Remove this hack | ||
| // Hack to ensure the ConnectEvm instance has fully resumed the connection | ||
| await new Promise((resolve) => setTimeout(resolve, 2000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this awful delay to ensure that this async call in the ConnectEvm constructor has fully resolved. We need to fix this on the connect-monorepo side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your understanding of how we resolve this on the connect-evm side of things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to release connect-evm again, bump the version in web3auth, and then remove this setTimeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| // if (options.autoConnect) { | ||
| // this.rehydrated = false; | ||
| // this.emit(CONNECTOR_EVENTS.REHYDRATION_ERROR, new Error("Failed to resume existing MetaMask Connect session.") as Web3AuthError); | ||
| // } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this commented code can be dropped?.. Not sure if it's more appropriate to throw an error if we fail to resume a previous session OR if we just say the connector is ready for new connections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think it makes more sense to just say the connector is ready for new connections. But perhaps we should speak with @chaitanyapotti to see what patterns they recommend for their connectors
packages/no-modal/src/connectors/metamask-connector/metamaskConnector.ts
Show resolved
Hide resolved
| /** | ||
| * Configuration options for the MetaMask connector using @metamask/connect-evm | ||
| */ | ||
| export interface MetaMaskConnectorSettings { | ||
| /** Dapp identification and branding settings */ | ||
| dapp?: { | ||
| name?: string; | ||
| url?: string; | ||
| }; | ||
| /** Enable debug logging for the MetaMask SDK */ | ||
| debug?: boolean; | ||
| } | ||
|
|
||
| export interface MetaMaskConnectorOptions extends BaseConnectorSettings { | ||
| connectorSettings?: Partial<MetaMaskSDKOptions>; | ||
| connectorSettings?: MetaMaskConnectorSettings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder to document this breaking change and include migration instructions where appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
| "eip155:5": "https://goerli.infura.io/v3/demo", | ||
| "eip155:11155111": "https://sepolia.infura.io/v3/demo", | ||
| "eip155:137": "https://polygon-rpc.com", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User-configured RPC URLs are ignored in SDK initialization
High Severity
The code builds a supportedNetworks map from user-configured chains (lines 142-148) but this variable is never passed to createEVMClient(). Instead, hardcoded RPC endpoints are used (lines 167-174), meaning users' custom chain configurations and RPC URLs are silently ignored. Only mainnet, goerli, sepolia, and polygon will work with the hardcoded endpoints. The dynamically built supportedNetworks variable appears intended for use but was left as dead code.
Summary
@metamask/sdkto@metamask/connect-evmfor improved connection handlingMetaMaskConnectorto use the new SDK'screateEVMClient()API with built-in event handlers and automatic chain managementswitchChain()Videos
Extension:
https://drive.google.com/file/d/1r_lDBvwHWQhP-J4yFfqVHsqRR7bIS6Z1/view?usp=sharing
Mobile (native browser):
https://drive.google.com/file/d/1XECLgCOqa9VEeIK4gp1nUf0cs6RRg2V1/view?usp=sharing
Note
Migrates EVM MetaMask integration to the new
@metamask/connect-evmSDK and wires it intoWeb3AuthNoModal.metamaskConnectorusingcreateEVMClient()with built-in event handlers, CAIP-2supportedNetworks,connect,disconnect, andswitchChain(auto add/switch)Web3AuthNoModal.loadConnectors()now addsmetaMaskConnector()for EIP-155 chains (preferred over injected)@web3auth/no-modalto10.13.2; replaces@metamask/sdkwith@metamask/connect-evminpackage.json; updates related deps/locksWritten by Cursor Bugbot for commit 87da4b7. This will update automatically on new commits. Configure here.