feat: update metamask sdk and implement connectSign #4323
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Do you you know if other wallets have the same features of connectAndSign and connectWith?
If the app only uses the MetaMask connector, it works.
But if they use multiple connectors, this means that in the Dapp code, the connector will need to be tested:
- If it’s MetaMask, then just call
connect. - If it’s not MetaMask, you need to call
connectand thensignMessage.
--
The best solution would be to propose a new connectAndSign hook to Wagmi, where the above logic would be wrapped. We could also add a new optional signMessage parameter to the connect method of the connector: https://wagmi.sh/dev/creating-connectors#methods
I'm available to discuss about it
The idea here is to have a least intrusive solution and allow the dapps using metamask to have a better U/X (no need for two deeplink for the mobile sign request) |
packages/connectors/src/metaMask.ts
Outdated
| | 'useDeeplink' | ||
| | 'injectProvider' | ||
| | 'forceInjectProvider' | ||
| | 'forceDeleteProvider' |
There was a problem hiding this comment.
Let's keep these around since otherwise it's a breaking change.
packages/connectors/src/metaMask.ts
Outdated
| > | ||
|
|
||
| export type MetaMaskParameters = WagmiMetaMaskSDKOptions & { | ||
| // Shortcut to connect and sign a message | ||
| connectAndSign?: string |
There was a problem hiding this comment.
| connectAndSign?: string | |
| connectAndSign?: string | undefined |
packages/connectors/src/metaMask.ts
Outdated
| // Shortcut to connect and sign a message | ||
| connectAndSign?: string | ||
| // Allow connectWith any rpc method | ||
| connectWith?: { method: string; params: unknown[] } |
There was a problem hiding this comment.
| connectWith?: { method: string; params: unknown[] } | |
| connectWith?: { method: string; params: unknown[] } | undefined |
What methods are supported? We should be able to strongly type this with the utility types in Viem (e.g. EIP1474Methods) if we want.
| // @ts-ignore | ||
| if (typeof SDK !== 'function' && typeof SDK.default === 'function') | ||
| // @ts-ignore | ||
| return SDK.default | ||
| // @ts-ignore |
There was a problem hiding this comment.
I'll take a crack on getting TS to play nice.
packages/connectors/src/metaMask.ts
Outdated
| dappMetadata: parameters.dappMetadata ?? { name: 'wagmi' }, | ||
| useDeeplink: parameters.useDeeplink ?? true, | ||
| dappMetadata: parameters.dappMetadata ?? { | ||
| url: `${window.location.protocol}//${window.location.host}`, |
There was a problem hiding this comment.
We should check that window is available (typeof window !== 'undefined')
| useDeeplink: true, | ||
| injectProvider: false, | ||
| forceInjectProvider: false, | ||
| forceDeleteProvider: false, |
There was a problem hiding this comment.
What were the defaults internally before? Just want to make sure changes these isn't too disruptive for folks. Probably should allow folks to override these manually so it's not a breaking change and remove that ability in the next major version. (Very unlikely anyone will override manually since people just use defaults, but want to make sure semver is respected.)
There was a problem hiding this comment.
We force inject provider but it can conflict with the injected provider by wagmi (see our other PR.)
There was a problem hiding this comment.
Is this PR dependent on that one? Haven't had a chance to wrap up our thinking on that quite yet.
|
Closing in favor of #4337 |
Update metamask sdk to 0.29.1