Skip to content

Add SubscribeToPayer/Identity, and use for wallet plugin#200

Draft
mcintyre94 wants to merge 1 commit into
wallet-pluginfrom
subscribe-to-reactive-capability
Draft

Add SubscribeToPayer/Identity, and use for wallet plugin#200
mcintyre94 wants to merge 1 commit into
wallet-pluginfrom
subscribe-to-reactive-capability

Conversation

@mcintyre94
Copy link
Copy Markdown
Member

@mcintyre94 mcintyre94 commented Apr 20, 2026

This PR adds new types ClientWithSubscribeToPayer and ClientWithSubscribeToIdentity. These allow a plugin (such as the wallet plugin) to notify subscribers when they modify these fields, without those consumers needing to know that a particular plugin has made them reactive.

It's already the case that client.payer can be made dynamic by the wallet plugin, by using a get() function. This retains compatibility with the payer: TransactionSigner type. But in order to make this useful for a reactive UI that may for example render based on the current value of client.payer, we also need to be able to subscribe to changes.

This PR wires up these subscribers in the wallet plugin, so that it notifies them when it updates the signer.

Reactive consumers can then use client.subscribeToPayer(() => {}) to receive these notifications.

Alternatives considered:

  • Add this to the wallet plugin only. This would work, but this would need to be duplicated by any other plugin that wants to make these fields reactive. In practice we'd be binding consumers (like a react library) to the wallet plugin's types. I think it makes more sense to define the types in kit-plugin-signer and allow consumers to be agnostic to how the fields are made dynamic
  • Make payer and identity actually act as a reactive store (with subscribe and getValue functions), this would be a breaking change and annoying for most uses. This PR provides the same functionality only to consumers that need to react to changes to these values. This is likely only reactive UI - anything using client.payer already gets the correct updated value

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 20, 2026

🦋 Changeset detected

Latest commit: 107a603

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@solana/kit-plugin-signer Minor
@solana/kit-plugin-wallet Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Copy Markdown
Member Author

mcintyre94 commented Apr 20, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@mcintyre94
Copy link
Copy Markdown
Member Author

@trevor-cortex

Copy link
Copy Markdown

@trevor-cortex trevor-cortex left a comment

Choose a reason for hiding this comment

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

Clean, well-scoped addition. The core design — defining the subscribeTo<Capability> convention in kit-plugin-signer so consumers can duck-type on it without binding to any specific plugin — is the right call, and the alternatives section in the PR description convinced me too. This looks good to me pending a couple of minor observations below.

What this does well

  • Filtering is done at the source on the signer identity itself (connected?.signer ?? null), so unrelated store notifications (wallet discovery, status transitions like pending → disconnected) don't spuriously wake listeners. That's the important correctness property and the tests verify it.
  • Each subscriber tracks its own last baseline, so an unsubscribe/re-subscribe cycle correctly resets. No shared mutable state between consumers.
  • Types are threaded through all three variants (walletSigner, walletPayer, walletIdentity) correctly, and walletWithoutSigner deliberately installs nothing — the type tests pin this down with @ts-expect-error.
  • Docblocks explicitly document over-notification as acceptable and point at useSyncExternalStore's snapshot-equality behavior as the backstop. Good expectation-setting for the contract.
  • Changeset is minor on both packages, which is correct for pre-1.0 feature additions under your convention.

Things for subsequent reviewers to verify

  • The store's listeners.forEach(l => l()) runs listeners synchronously; a throwing user listener would still propagate and potentially skip siblings. That's pre-existing behavior from store.ts and not introduced here, but worth being aware of now that subscribe hooks are exposed as a public API surface.
  • On walletSigner, both subscribeToPayer and subscribeToIdentity subscribe to the same underlying signer identity, so a single account switch fires both listeners — which matches the contract since both client.payer and client.identity resolve to the same signer. The walletSigner fires both subscriptions test covers this.
  • No test currently exercises selectAccount firing the listener, but the filter is purely on signer reference identity so it should behave correctly by construction.
  • The convention is structural: anything shaped like { subscribeToPayer: (listener) => () => void } satisfies ClientWithSubscribeToPayer. That's intentional per the docblock — worth keeping in mind if a future plugin wants to opt into the same contract.

Minor nit: the changeset file is missing a trailing newline (\ No newline at end of file in the diff). Not worth blocking on; Prettier or a follow-up will catch it.

last = curr;
listener();
}
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small observation, no change requested: the subscribe factory is allocated per-property inside the loop. With only two capabilities that's fine, but if the signerProperties list ever grows or if you add more subscribeTo* capabilities, it might be worth hoisting this into a named helper (e.g. createSignerChangeSubscriber(store)) so the branching and closure allocation live in one place. Purely stylistic.

*
* Calling the returned unsubscribe more than once is safe — it must be
* idempotent.
*/
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The docblock guarantees idempotent unsubscribe ("Calling the returned unsubscribe more than once is safe"). The current wallet-plugin implementation delegates to store.subscribe, whose unsubscribe is a listeners.delete(listener) — naturally idempotent. Worth noting that any future plugin adopting this convention needs to uphold that contract; consider whether you want a type-level or test-level marker to make that obvious. Not blocking.

unsubscribe();
await client.wallet.disconnect();
expect(listener).toHaveBeenCalledTimes(1);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider adding a test that selectAccount (switching to a different account on the same wallet) fires the listener — it exercises a code path distinct from connect/disconnect and would lock in the "fires on any signer identity change" contract. Not blocking; the implementation filters on signer reference equality so it should Just Work.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea, added this test

@mcintyre94 mcintyre94 force-pushed the subscribe-to-reactive-capability branch from 7808e13 to f821c81 Compare April 20, 2026 10:58
@mcintyre94 mcintyre94 requested a review from lorisleiva April 20, 2026 11:01
@mcintyre94 mcintyre94 marked this pull request as ready for review April 20, 2026 11:02
@mcintyre94 mcintyre94 marked this pull request as draft April 21, 2026 10:42
@mcintyre94
Copy link
Copy Markdown
Member Author

Draft for now. Types moved to anza-xyz/kit#1551, this PR would just use them in the wallet plugin. Will require a Kit release + dependency bump here

@mcintyre94 mcintyre94 force-pushed the subscribe-to-reactive-capability branch 2 times, most recently from 02d3f6f to 9940096 Compare April 23, 2026 12:04
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.

2 participants