-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: solana support #123
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: main
Are you sure you want to change the base?
feat: solana support #123
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
|
@SocketSecurity ignore npm/@solana/wallet-standard-chains@1.1.1
|
| const core = await createMultichainClient({ | ||
| dapp: options.dapp, | ||
| api: { | ||
| supportedNetworks: options.api?.supportedNetworks ?? {}, |
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.
Maybe we add mainnet as the default?
| supportedNetworks: options.api?.supportedNetworks ?? {}, | |
| supportedNetworks: options.api?.supportedNetworks ?? { | |
| 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp': 'https://api.mainnet-beta.solana.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.
IMO we should not expect the solana namespace to be included in the callers options.api.supportedNetworks key values
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.
^ in reference to how there is no massaging going on here. Not about Donesky's comment.
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 you might need to be a bit more clear about your followup comments here Jiexi I don't think I fully understand
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 I understand is that'd be preferable to just receive the RPC URL, perhaps mapped to one of mainnet, devnet, testnet instead of the Solana namespace?
Something like:
{ 'mainnet': 'https://api.mainnet-beta.solana.com' }
And we do the namespace mapping ourselves. WDYT?
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.
Here's what I'm suggesting: ec248c8
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.
ahh sorry for the ambiguity.
I mean that this logic might look more like
// no `solana:` prefix
options.api.supportedNetworks = {
'5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp': 'https://api.mainnet-beta.solana.com'
}
const supportedNetworksWithSolanaNamespace = Object.fromEntries(
Object.entries(originalObject).map(([key, value]) => [
`solana:${key}`,
value
])
)
...
{
supportedNetworks: supportedNetworksWithSolanaNamespace
}
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.e. supportedNetworks is expected just to be the genesis hash (which identifies the network/chain) without the solana: prefix which we will add ourselves before passing it to createMultichainClient
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.
yeah I suppose this makes sense though we currently for connect-evm we use the full caip chainIds ->
| supportedNetworks: options.api.supportedNetworks, |
&
connect-monorepo/packages/connect-multichain/src/domain/multichain/api/types.ts
Lines 81 to 85 in ec248c8
| export type RpcUrlsMap = { | |
| /** CAIP-2 format chain ID mapped to its RPC URL (e.g., "eip155:1" -> "https://...") */ | |
| [chainId: CaipChainId]: string; | |
| }; | |
packages/connect-solana/package.json
Outdated
| @@ -0,0 +1,80 @@ | |||
| { | |||
| "name": "@metamask/connect-solana", | |||
| "version": "0.1.0", | |||
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.
this needs to start at 0.0.0
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.
| export type SolanaClient = { | ||
| /** The underlying MultichainCore instance */ | ||
| core: MultichainCore; | ||
| /** | ||
| * Gets a wallet-standard compatible wallet instance. | ||
| * @param walletName - Optional custom name for the wallet | ||
| * @returns The wallet instance | ||
| */ | ||
| getWallet: (walletName?: string) => Wallet; | ||
| /** | ||
| * Registers the MetaMask wallet with the wallet-standard registry. | ||
| * This makes MetaMask automatically discoverable by Solana dapps. | ||
| * @param walletName - Optional custom name for the wallet | ||
| */ | ||
| registerWallet: (walletName?: string) => Promise<void>; | ||
| /** | ||
| * Disconnects from the wallet and revokes the session. | ||
| */ | ||
| disconnect: () => Promise<void>; | ||
| }; |
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.
the EVM and Solana Clients are pretty different at this point. I wonder if we should try to establish a common pattern. For example should we remove the request() method from ConnectEvm and make the 1193 provider the only way to make requests like we are kind of doing here? (i know MultichainCore is exposed though)
packages/connect-solana/CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - Initial release ([#123](https://github.com/MetaMask/connect-monorepo/pull/123)) |
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.
technically we aren't releasing it yet. sorry to nit pick lol
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.
| * This makes MetaMask automatically discoverable by Solana dapps. | ||
| * @param walletName - Optional custom name for the wallet | ||
| */ | ||
| registerWallet: (walletName?: string) => Promise<void>; |
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 should align on if MMC or the dapp is responsible for determining if an announcement event should be made here and for EVM
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.
Think we're going to need to sync on this one as well @jiexi, not sure I follow
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.
Right now it is the dapp's responsibility to call registerWallet() in order to make the MMC created provider discoverable rather than MMC automatically announcing itself. Does this seem like the right pattern? I was more on the of side of MMC automatically announcing itself being better, but kind of changing my mind now.
tldr; nevermind this is good!
packages/connect-solana/src/types.ts
Outdated
| /** | ||
| * Configuration options for creating a Solana client. | ||
| */ | ||
| export type SolanaConnectOptions = { |
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.
should this be derived more directly from ConnectMultichain types instead like we do for the EVM client?
Pick<MultichainOptions, 'dapp' | 'api'> &
{ ui?: Omit<MultichainOptions['ui'], 'factory'>; } & {
eventHandlers?: Partial<EventHandlers>;
debug?: boolean;
}
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.
packages/connect-solana/src/types.ts
Outdated
| }; | ||
| /** Optional API configuration */ | ||
| api?: { | ||
| /** A map of CAIP chain IDs to RPC URLs for supported networks */ |
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.
should this be Solana Genesis Hash to RPC URLs?
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.
🤔 also not following this one haha
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.
why are the changes here needed? Sorry if obvious 🙏
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.
-
Swapped
'require'before'import'to prefer CommonJS over ESM. This is likely because some Solana packages (like@solana/web3.jsor@metamask/solana-wallet-standard) have dual CJS/ESM exports, and their ESM builds may have compatibility issues with the Create React App / webpack setup. Preferring CommonJS avoids those issues. -
This disables webpack's "fully specified" requirement for
.mjsfiles fromnode_modules. Without this, webpack would require imports likeimport foo from './bar.js'(with extension), but many ESM packages useimport foo from './bar'(without extension). Solana ecosystem packages often trigger this issue.
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.
Tried unswapping these to resolve this issue but it didn't help 😢
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.
From Claude re this issue
Three versions of bs58 → base-x:
| bs58 version | base-x version | Pulled in by |
|---|---|---|
| 4.0.1 | 3.0.11 (CJS only) | @solana/web3.js, borsh |
| 5.0.0 | 4.0.1 | @solana-mobile/* packages |
| 6.0.0 | 5.0.1 (ESM + CJS) | @metamask/solana-wallet-standard, @reown/appkit, @walletconnect/utils |
Why this causes the error:
-
Webpack hoists the oldest version -
base-x@3.0.11gets installed at the rootnode_modules/base-x/because it was likely resolved first or has the most dependents. -
Nested versions exist but webpack ignores them -
base-x@5.0.1is nested insidenode_modules/bs58/node_modules/base-x/, but when webpack seesimport basex from 'base-x', it resolves from the root, not the nested location. -
ESM/CJS mismatch -
bs58@6.0.0(used by your Solana wallet adapter chain) has ESM code that does:import basex from 'base-x'; export default basex(ALPHABET); // expects basex to be a function
But
base-x@3.0.11(the hoisted version) is CJS-only with a different export structure, sobasexisn't a function.
package.json
Outdated
| "yargs": "^17.7.2" | ||
| }, | ||
| "resolutions": { | ||
| "@metamask/utils": "^11.8.1", |
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.
why did this get moved to the root level package.json?
and why the bs58 forced resolution?
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.
both were (unsafe) attempts at reducing bundle size. I've removed them. 9e771ff
| expect(client.core).toBe(mockCore); | ||
| }); | ||
|
|
||
| it('should get wallet using getWalletStandard', async () => { |
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.
thoughts on moving the tests starting from here and down into their own scenarios? everything above this is definitely more related to createSolanaClient but i don't think the ones starting here and below are
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.
| try { | ||
| const encodedMessage = new TextEncoder().encode(message); | ||
| const signature = await signMessage(encodedMessage); | ||
| setSignedMessage(Buffer.from(signature).toString('base64')); |
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.
doesn't really matter but is there a way to do this without Buffer?
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.
again, doesn't really matter though
| const connectSolana = useCallback(async () => { | ||
| // Find the MetaMask wallet in registered wallets | ||
| const metamaskWallet = wallets.find((w) => | ||
| w.adapter.name.toLowerCase().includes('metamask') |
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.
is there a chance this will perfer the extension announced event instead of the ConnectSolana one?

Explanation
Introduces
@metamask/connect-solana, a new package that wraps@metamask/solana-wallet-standardwith MetaMask Connect, enabling Solana dapps to use MetaMask via the wallet-standard protocol. It also adds playground support for solana-wallet-standard which enables testing this feature.Testing Steps
yarn install && yarn build)yarn workspace @metamask/browser-playground start)Connect (Solana)button (ensure no extension is installed if using desktop browser)References
Fixes WAPI-751
Checklist