Skip to content

Conversation

@wenfix
Copy link
Owner

@wenfix wenfix commented Nov 11, 2025

Setup:

  1. Pull the latest connect-monorepo and yarn && yarn build
  2. Clone this repo and checkout to this branch (feat/metamask-connector-upgrade)
  3. pnpm i to install deps
  4. pnpm run dev:react to launch the playground

isReconnecting?: boolean | undefined
withCapabilities?: withCapabilities | boolean | undefined
}) {
// TODO: better handling when not providing a chainId?
Copy link
Collaborator

Choose a reason for hiding this comment

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

What could we consider as "better handling" ?
I believe defaulting to mainnet seems like a good approach here

try {
const result = await metamask.connect({
chainId: chainId,
account: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to explicitly set account to undefined.

If not, think it's cleaner to omit it altogether and just have

        const result = await metamask.connect({  chainId: chainId });

async switchChain({ addEthereumChainParameter, chainId }) {
const provider = await this.getProvider()

async switchChain({ addEthereumChainParameter, chainId }) {
Copy link
Collaborator

@ffmcgee725 ffmcgee725 Nov 19, 2025

Choose a reason for hiding this comment

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

Not suggesting anything in particular, but want to understand
do you remember why we are now deviating from doing

       await provider.request({
          method: 'wallet_switchEthereumChain',
          params: [{ chainId: numberToHex(chainId) }],
        })

like before ?

EDIT: as I read further, perhaps the call to

await metamask.switchChain({ chainId, chainConfiguration })

handles this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants