Conversation
|
d88512c to
2435b0e
Compare
amilz
left a comment
There was a problem hiding this comment.
Awesome. Great work w/ this!
| "./src/" | ||
| ], | ||
| "sideEffects": false, | ||
| "keywords": [ |
There was a problem hiding this comment.
Not sure how much these matter but might make sese to add "wallet-adapter" since this is a big piece of migrating from wallet adapter
| for (const [key, fn] of Object.entries({ | ||
| connected: () => store.getConnected(), | ||
| status: () => store.getState().status, | ||
| wallets: () => store.getState().wallets, |
There was a problem hiding this comment.
wdyt about calling this detected or discovered or something like that? Fine as-is, but wallet.wallets just feels a little odd.
There was a problem hiding this comment.
🎶 Can I get a... wallet.wallets? Literally the first thing my eye jumped to when reading the PR description. Reminds me of the metadata.data.data days haha.
More seriously though, that type of naming is hard. Even wallet.detected or wallet.discoved doesn't work because the singular wallet entrypoint doesn't convey that discovered is an array of wallets.
IMO there are two approaches to fix it: we either change the entrypoint terminology or make the attribute more specific/verbose.
For the former, we could coin this whole plugin as something different than wallet. For example, a program repository plugin could be coined as the Librarian plugin. Here it's a bit more tricky but we could go with things like Locksmith or Keyring.
That being said, Wallet is a very intuitive terminology (Apple wallet, etc.) so the second option might make more sense. We could go:
wallet.all: The easy path, but it's maybe not super clear whatallmeans.wallet.detectedWallets: The longer path, we still have wallet-wallet but it's more justified.wallet.allDetected: A hybrid.wallet.cards: The braver path, we brand wallets as cards (or similar) to go deeper into the analogy and have more explicit naming conventions.
There was a problem hiding this comment.
I'm hesitant to introduce any new terminology here, which is why I went with wallet for the plugin namespace, and wallets mirroring useWallets from wallet-adapter here.
I think detectedWallets is probably the best option TBH.
One point just to add some context is that client.wallet.wallets is not how most apps will actually consume this:
// react
function useWalletState(): WalletStateSnapshot {
return useSyncExternalStore(client.wallet.subscribe, client.wallet.getSnapshot);
}
const { wallets, connected, status } = useWalletState();// svelte
export const walletState = readable(client.wallet.getSnapshot(), (set) => {
return client.wallet.subscribe(() => set(client.wallet.getSnapshot()));
});
{#each $walletState.wallets as w (w.name)}// vanilla, but reading the snapshot in a function called on `subscribe`
const { wallets, connected, status } = client.wallet.getSnapshot();I'll take a proper look at this tomorrow, but I think I'm leaning toward stripping some of this state off client.wallet at all, and forcing apps to read through the snapshot. That does mean this plugin leans more into assuming you'll either have a reactive framework on top, or are comfortable with a snapshot/subscribe API. But I think that might be the better design here.
I think we'd drop wallet.wallets and wallet.status, and strip wallet.connected to just wallet.signer just because I'm trying to keep the signer out of the snapshot. Everything else is in the snapshot and apps should, in practice, read it from there. Having it at the top level is probably just confusing.
There was a problem hiding this comment.
I agree that having multiple access points to these values is probably more hurting than improving the DevEx here.
I think I noticed is connected seems to be both used as a connection check and the actual connected wallet. I don't think connected is very clear on its own that it's anything other than a boolean so perhaps connectedWallet is better if we were to keep that variable somewhere.
Unpolished idea regarding client.wallet.signer. What if client.wallet itself became the signer when available. You'd have an address getting on it and any signing functions available on the signer. The case where a signer is not yet connected might be tricky to deal with but worth exploring? Maybe that's useless though since we already set client.payer anyway.
There was a problem hiding this comment.
After a bit more back and forth here, I think we can probably pull signer into the snapshot. I was thinking about it too much from a react perspective, but actually at this level we control memoization, and we control when we signal the listeners. I think we have enough information here to know when we actually need to re-create the signer, and to memoize it. So at that point all state is only in the snapshot.
I do agree that connected isn't great as a name, but the idea (if we include signer) is that it contains the {wallet: UiWallet, account: UiWalletAccount, signer?: WalletSigner}
And then connected.wallet, connected.account, connected.signer reads quite nicely I think. We could rename it, but I think we should try to make it read well primarily when used like that because I think that'll be the most common usage.
We could also add isConnected if we want a clean boolean property, easy to keep in sync. Though I think a lot of apps would end up using connected for the nullish coalescing: connected?.wallet.
There was a problem hiding this comment.
Oh I see, yeah in think connected.signer reads quite well tbf.
| await client.wallet.connect(selectedWallet); | ||
|
|
||
| // client.payer is now the connected wallet's signer | ||
| await client.sendTransaction( |
There was a problem hiding this comment.
Will this work w/o adding txn planner/executors?
There was a problem hiding this comment.
Yes - you get a TransactionSigner and can use it with any Kit APIs.
I did mean to flag the decision not to have client.wallet.signTransaction() though. I think we should just use the signer APIs and also offering signTransaction would be confusing.
I think most apps will want to just use their transaction plan/execute plugin
If you don't, you can just add client.wallet.connected.signer (will probably move a little but same idea) as a signer on the transaction and sign/send it however you like
At the lowest level you have client.wallet.connected.signer.signAndModifyTransaction([...]) which is basically the same API we'd be offering as client.signTransaction, but just the signer interface
There was a problem hiding this comment.
I agree with this approach. We even have helpers like signTransactionWithSigners(signers, tx) now so fully leaning into the Signer API makes total sense to me.
| - A subscribable store that any framework can bind to | ||
| - Cleanup via `withCleanup` and `Symbol.dispose` | ||
|
|
||
| ### Where it fits |
wallet-plugin-spec.md
Outdated
|
|
||
| ### Relationship to `payer` | ||
|
|
||
| The `wallet` plugin and `payer` plugin both set `client.payer`. Use `payer()` for static signers (backend keypairs, scripts) and `wallet()` for dApps with user-facing wallet connections. |
There was a problem hiding this comment.
Is this a common use case where ppl would need access to both? Seems easy enough to me to createWalletClient for web stuff and createBackendClient for doing server signing or whatever is needed? Happy to discuss.
There was a problem hiding this comment.
The case this is really trying to cover is something like createLocalClient().use(wallet()). Here the local client is under the hood including a default payer, and then we're overwriting it.
We don't want you to have to unbundle createLocalClient() to use the wallet plugin. And we don't want (debatable, maybe we do) to break client.payer just because you have the wallet plugin loaded and no wallet connected.
In general I think it definitely is valid to have different clients for the server and client, and I think SSR is the only reason to have the wallet plugin on the server.
I think allowing payer and wallet is essential (for eg that first case), but we could strip out the payer fallback handling if we don't think that's important. You'd just decide if you want the wallet to control payer - if you do then it's connectedWalletAsSigner | null, if you don't then it's left as-is.
There was a problem hiding this comment.
That said... I've just realised that doesn't work correctly because of what we discussed earlier:
createLocalClient()
.use(wallet())
.use(systemProgram())Now client.system.instructions.whatever().sendTransaction() is going to use the transaction planner on createLocalClient, which was set before the wallet plugin, and is going to use the payer from createLocalClient.
Even though we declare system program after wallet, its dependency came before wallet.
At that point you are needing to unbundle to put the wallet plugin before rpcTransactionPlanner and having payer and then wallet is kinda questionable.
You also see this with the non-local rpc client:
createClient({payer: await generateKeyPairSigner()}) // payer is required but we don't really have one...
.use(wallet())
.use(systemProgram())Now the planner has closed over a randomly generated client.payer with no funds as its default.
Maybe we do need to just accept that this is going to need to be used in a createBrowserClient or similar, and can't just be added after a different client - at least when you want it to control payer. At that point we might as well use this instead of payer, before rpcTransactionPlanner. And then there's probably no need for the fallback being on the wallet plugin.
There was a problem hiding this comment.
Okay so... if the API here is that this is part of your initial client (rather than tagged onto the end of an existing one) by necessity, then we should probably think about the payer at that level. And then design the plugin to fit and support the necessary use cases.
- the wallet is not the payer, eg a gasless app. You'd set a
payerplugin on that client, and the wallet wouldn't be used. - the wallet is the payer, it pays fees. You wouldn't set a
payerplugin on that client, and the wallet would be used. - the wallet is the payer, if there is one. Otherwise you have a fallback payer that your app will use to send any transactions it needs to. Maybe we don't need this case? If we do, then you could either:
a) keep the current design. you'd put apayerfirst with the fallback payer, then thewalletconfigured withusePayer: true
b) nopayerplugin, modify the design so the wallet just receives a fallback payer that it uses (instead of reading it from the input client)
I think I'm leaning toward case 3 being more confusing than useful, which also simplifies the plugin. If we only support cases 1 and 2, then the client config could be something like {payer: TransactionSigner | 'wallet'}. If you pass a TransactionSigner then we add the payer plugin with that signer, and the wallet plugin configured not to set the payer. If you pass wallet then we do not add the payer plugin, and the wallet plugin is configured to set the payer to the connected wallet.
The wallet plugin loses the ability to use a fallback payer, and will just set payer: undefined when it controls the payer and there is not a connected tx signing wallet. The client implements Partial<ClientWithPayer>, which is unchanged.
So the config on the wallet is still just usePayer, which per your other comment I think we should just make that required config so it's explicit. And maybe rename it useAsPayer.
We end up with something like (pseudocode):
function createWalletRpcClient(config) {
return createEmptyClient()
.use(rpc(config.url, config.rpcSubscriptionsConfig))
// skip payer if we're using wallet as payer
.use(config.payer !== 'wallet' ? payer(configuredPayer) : c => c)
.use(wallet({ useAsPayer: config.payer === 'wallet', chain: config.solanaChain }))
.use(rpcGetMinimumBalance())
.use(rpcTransactionPlanner({ priorityFees: 0 }))
.use(rpcTransactionPlanExecutor({ maxConcurrency: 5, skipPreflight: false }))
.use(planAndSendTransactions());
}There was a problem hiding this comment.
One more design step here, I wonder if we should actually have two plugins, wallet and walletAsPayer. That way we can expose the distinction at the type-level and not mess with the type when we don't need to. Eg:
const client = createEmptyClient()
.use(payer(mySigner)
.use(wallet())
// client.payer is TransactionSignerconst client = createEmptyClient()
.use(wallet())
// client.payer is type errorcreateEmptyClient()
.use(walletAsPayer())
// client.payer is TransactionSigner | undefinedThere was a problem hiding this comment.
I think there are valid cases for wanting to have client.payer not be related to the user's wallet, eg making your app gasless but still having users sign transactions.
One example that comes to mind is Kora, where you wouldn't want the user's wallet to override the gasless feepayer:
const client = createClient()
.use(kora(config)) // plugin bundle that sets Kora payer, rpc, custom tx plan + execute
.use(wallet()This would enable a gasless dapp UI, where the custom transaction planner has the user paying fees in eg USDC, but the transaction fee payer is always the Kora signer
I agree with not shadowing client.payer and falling back, that was a bad design. But I do think it's valid to need client.wallet but need it not to be the fee payer.
I guess if we made wallet always set the payer we could do:
const client = createClient()
.use(wallet()) // sets wallet, payer
.use(kora(config)) // plugin bundle that sets Kora payer, rpc, custom tx plan + executeBut that'd only work if the Kora plugin bundle always overrides the payer, which would probably make sense for that use case.
But then you couldn't use a plugin bundle that doesn't override, which is how we said rpcClient would behave:
const client = createClient()
.use(wallet()) // sets wallet, payer
.use(rpcClient({ payer: serverSigner }) // gasless app without Kora. Does `rpcClient` skip setting payer because there's already one?There was a problem hiding this comment.
This is literally the problem client.identity tries to solve. client.payer is the thing that pays for fees and client.identity is the thing that uses the app and owns the data. Most of the time they are the same but there are legit use-cases where they are not.
Maybe it's time to seriously think about client.identity in the plugin system?
There was a problem hiding this comment.
I'd agree with that. wallet would always control client.identity and walletAsPayer would also control client.payer. There's maybe an interesting distinction - payer always needs to be a signer, but arguably client.identity could just be an address?
If we used this then program plugins could default the connected wallet for instructions with the identity node, which would be really nice. Though some of them would probably require a signer.
There was a problem hiding this comment.
Hmm client.identity would likely need to sign for various things even if it's not payer right? For instance, if it wants to send an NFT to someone as the owner, client.payer maybe be payer for transaction/storage fees but client.identity must be the one approving the transfer.
I think to design these identity vs payer setters we need to think about a simpler example. For instance, use(generatedPayer()) from kit-plugin-payer (currently) will probably need to become something like:
use(generatedSigner())-> generated a signer and sets bothclient.identityandclient.payer.use(generatedPayer())-> payer only.use(generatedIdentity())-> identity only.
Then we'd only document the generatedSigner versions since that's what people will need 95% of the time but we'd be able to explain to users that they can split that into their payer/identity variants for more granular control. Also, we'd likely need to rename kit-plugin-payer to kit-plugin-signer or something similar.
Wdyt?
There was a problem hiding this comment.
I see yep, makes sense to make it a signer. Makes it work the same as payer in walletWithPayer too so should be straightforward. Agree with generated signer/payer/identity too.
Do you think we can do this orthogonal to the wallet plugin? Ie later enhance the wallet plugin to set client.identity to the connected wallet.
Or do you think we need to figure out client.identity first and wallet doesn't make sense without it?
IMO most of the value is still there without client.identity and it's additive, but not sure if you'd agree with that.
There was a problem hiding this comment.
Make sure to update root README & CONTRIBUTING
There was a problem hiding this comment.
I think these should be done after we have the actual implementation etc and it's ready to use, but agreed!
| - `client.wallet.selectAccount(account)` — Switch to a different account within an already-authorized wallet without reconnecting. | ||
|
|
||
| ```ts | ||
| client.wallet.selectAccount(selectedWallet); |
There was a problem hiding this comment.
I think this should be account w/i the accounts returned from connected??
| client.wallet.selectAccount(selectedWallet); | |
| client.wallet.selectAccount(accounts[0]); |
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
This PR scaffolds the @solana/kit-plugin-wallet package — a framework-agnostic wallet plugin that manages wallet discovery, connection lifecycle, signer creation, and payer integration via wallet-standard. The implementation is stub-only (createWalletStore throws not implemented), so this is purely the type system, API surface, configuration design, and package scaffolding.
The design is very well thought out. The API surface is clean, the subscribe/getSnapshot pattern for framework integration is the right call, and the SSR story with pending status is elegant. The spec included in wallet-plugin-spec.md provides excellent context for the implementation that will follow.
Key observations
Things that look great
hasSignerin snapshot vssignerinWalletConnection— smart separation to avoid unnecessary re-renders while still exposing the actual signer for instruction building.pendingvsdisconnecteddistinction — avoids the UI flash problem on SSR/auto-reconnect. This is a common pain point in wallet adapters.usePayerwith fallback capture — the dynamic getter approach with fallback to the previousclient.payeris clean. Plugin ordering (payerbeforewallet) is well-documented.WalletStorageinterface — supporting async backends while being compatible withlocalStorage/sessionStoragedirectly is practical.signInoverloads — the two-argument form (SIWS-as-connect) is a nice ergonomic touch.
Things to watch during implementation
- Race conditions in
connect/disconnect— concurrent calls (user spam-clicks connect, or connects during auto-reconnect) need careful state machine transitions. The spec mentions this but it'll be the trickiest part ofcreateWalletStore. - Signer caching invalidation — when a wallet's accounts change externally (via
standard:events), cached signers for removed accounts need cleanup. TheWalletStoreStatehas a singlesignerfield, so this should be straightforward, but worth verifying during implementation. fallbackPayercapture timing — the spec shows capturingclient.payerat plugin install time. If the prior payer is itself a dynamic getter (from another plugin), you'll want to capture the descriptor, not the value, to preserve the getter chain. Or at least document that the fallback is evaluated once.
Minor items
See inline comments below.
Notes for subsequent reviewers
- The
wallet-plugin-spec.md(1299 lines) is context-only and not intended for permanent inclusion in the repo — author has noted this. - The
createWalletStorefunction is the heart of this plugin and is entirely stubbed. The real review work comes when that's implemented. @solana/kitis listed as both apeerDependencyand resolved as a regular dependency in the lockfile — worth checking if this is intentional or if it should only be a peer dep.- The
extendClientimport from@solana/kitis present but unused in the current scaffold (the plugin function implementation is truncated in the diff, but the store is a stub anyway).
| "test:treeshakability": "for file in dist/index.*.mjs; do agadoo $file; done", | ||
| "test:types": "tsc --noEmit" | ||
| }, | ||
| "peerDependencies": { |
There was a problem hiding this comment.
@solana/kit appears as both a peer dependency here and as a resolved regular dependency in the lockfile. Was the intent for it to be peer-only? If so, it might be getting hoisted from the workspace root. If it's intentionally in both, the ^6.5.0 range in peer deps is fine, but having it resolved in the lockfile as a direct dep could cause issues for consumers who have a different minor version.
| ```ts | ||
| client.wallet.selectAccount(selectedWallet); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
client.wallet.selectAccount(selectedWallet);This example passes selectedWallet (a UiWallet) but selectAccount takes a UiWalletAccount. Should be something like:
const accounts = await client.wallet.connect(selectedWallet);
client.wallet.selectAccount(accounts[1]); // switch to second account| @@ -0,0 +1,7 @@ | |||
| { | |||
| "$schema": "https://json.schemastore.org/tsconfig", | |||
| "compilerOptions": { "lib": [] }, | |||
There was a problem hiding this comment.
"lib": [] — this is presumably overridden by the base tsconfig, but an empty lib array with no override would mean no DOM types, which you'll need for localStorage, window, etc. in the implementation. Worth verifying the base config provides what you need, or explicitly adding "DOM" here when the implementation lands.
486470f to
9ff1b02
Compare
- All state is now exclusively stored on the snapshot, including the signer - Fallback payer is no longer used - Split into 2 plugins, `wallet` which does not touch payer, and `walletAsPayer` which replaces `client.payer` with the selected wallet
9ff1b02 to
ca6a002
Compare
|
I've updated this PR with a new commit: ca6a002 This is a large update and addresses a lot of what we've talked about:
I've also updated the description, and the demos - which already mostly used the reactive subscribe/snapshot APIs so have minimal changes. Also dependabot bumped us to Kit 6.6.0, so I've updated that dependency here and used |
| return { | ||
| subscribe: (l: () => void) => store.subscribe(l), | ||
| getSnapshot: () => store.getSnapshot(), | ||
| connect: (w: UiWallet) => store.connect(w), | ||
| disconnect: () => store.disconnect(), | ||
| selectAccount: (a: UiWalletAccount) => store.selectAccount(a), | ||
| signMessage: (msg: Uint8Array) => store.signMessage(msg), | ||
| signIn: (...args: [SolanaSignInInput?] | [UiWallet, SolanaSignInInput?]) => store.signIn(...args), | ||
| }; |
There was a problem hiding this comment.
I must be missing something but is this not the same as return store?
There was a problem hiding this comment.
Minus a couple of functions yep. A simpler design would be to structure the store so all of these are nested under store.public or something, and then just return that.
Caveat on the spec is that it's the result of back and forth with Claude web, which I don't think has the same refinement cycle as Claude Code. A lot of the code here can likely be simplified at implementation time
There was a problem hiding this comment.
Gotcha, I was just confused because wallet and store seemed to be different things but they are actually the same concepts.
I wonder if it would make sense to have client.walletStore for all that stuff and have client.wallet essentially become a shortcut for client.walletStore.getState().connected.
There was a problem hiding this comment.
Yea I think store was originally internal implementation and quite different to wallet, but they've converged. I think aftere renaming getSnapshot() to getState() and removing the current store.getState() that I don't think we need additional, the only thing on store and not wallet is the destroy function, so we could just set wallet: store. Probably a good sign that the internal implementation is as simple as the interface now!
If we made client.wallet point at connected, what would be our interface to subscribe/connect etc? Currently that's client.wallet.subscribe etc.
There was a problem hiding this comment.
That's fair yeah probably best to keep client.wallet as-is then. Especially if we're gonna rely on client.payer and client.identity as signer entrypoints.
This PR adds the scaffolding and design of the proposed wallet plugin, without yet implementing it
The plugin is designed to expose the interface expected by reactive frameworks, eg React's useSyncExternalStore.
Example (Vanilla JS)
Key functionality
standard:connectfeature, as well as an optionalfilterthat can be passed to the plugin. ReturnsUiWallet[]connect(wallet: UiWallet), we derive aSignerfor the connected wallet account. This will include all available signer features of modify + send tx, sign + send tx, modify + sign message. See Kit function: https://github.com/anza-xyz/kit/blob/f055201c2dd3a4a69b9894d66b622ae81c13b8cd/packages/wallet-account-signer/src/wallet-account-signer.ts#L68. If the wallet account is read-only, we don't set the signer but do set wallet + accountwalletandwalletAsPayer. Both provide an identicalclient.walletinterface. ThewalletAsPayerplugin additionally setsclient.payerto the connected wallet.storage: null, and is always disabled on the server.autoConnectcan be independently disabled while using storage too.getSnapshot()andsubscribe()APIs.getSnapshotreturns a referentially stable object describing the state, andsubscribeallows registering listeners that are notified whenever state changes. Together these enable a reactive store like React'suseSyncExternalStore, Svelte'sreadableand Vue's reactive state.pendingstate, and all functions will throw. But consumers can safely include it in their SSR pass for simplicity. Thependingstate is intentionally distinct fromdisconnectedto allow eg not rendering/rendering a skeleton and avoid a UI flash from disconnected -> connectedImplementation notes/TODOs
We haven't published Kit since merging
withCleanup, so for now this plugin implements[Symbol.Dispose]directly.I think we will need one new error added to Kit. For now this is just included as an error class in the plugin. I think it's justifiable in Kit because it'd be useful to any similar wallet functionality.
ClientWithWalletandClientWithWalletAsPayerto kit plugin-interfaces later, and move some of the types thereMore examples
I've had Claude generate a simple example app for React, Svelte, Vue and Vanilla JS: https://github.com/anza-xyz/kit-plugins/tree/wallet-plugin-demos-1/examples/wallet-demo/src . These are intended to show how different frameworks can bind to the state in this plugin and expose it to apps. They are not intended to be representative of how actual apps would be written with kit-plugins, but hopefully show how thin the framework-specific layer for wallets can be. One of my design goals is to never again upset someone who doesn't want to use React!
For full transparency, this PR includes a spec file. This is long and the result of lots of back and forth with an LLM, I don't expect anyone to review it and will later remove it from the repo. But wanted to include it here to give more context about how this will all work!