Repository navigation
Conversation
✅ Deploy Preview for sats-connect canceled.
|
victorkirov
left a comment
There was a problem hiding this comment.
Automated review (round 1): two blocking issues. @sats-connect/core is pinned to an unmerged prerelease with no release guard, and Wallet.listen throws on every wallet that doesn't support SIP-030 yet while the documented examples don't handle it. Suggestions are inline.
| public listen: Listen = (event, callback) => { | ||
| const providerId = this.providerId ?? getDefaultProvider(); | ||
| if (!providerId) { | ||
| throw new Error('Select a wallet provider before registering SIP-030 listeners.'); |
There was a problem hiding this comment.
[P2] Wallet.listen throws on wallets without SIP-030 support, and the documented examples don't handle it
Wallet.listen throws synchronously when no provider is selected, and Core throws when the provider has no native listen. That covers every Xverse version shipped before this rollout, plus the Unisat and Fordefi adapters. The sibling addListener deliberately logs and returns a no-op so that apps don't crash when sats-connect is ahead of the installed wallet (see its comment). The SIP030_LISTENERS.md examples and the README call Wallet.listen with no try/catch, so an app that registers on mount (in a React effect or at page init) will throw for first-time visitors and for users on older wallets. Smallest fix: either match addListener (console.error and return () => {}), or keep the throw, document it, and show try/catch or a capability check in the examples.
| } | ||
| this.providerId = providerId; | ||
|
|
||
| const Adapter = this.defaultAdapters[providerId]; |
There was a problem hiding this comment.
[P3] Adapter listen dispatch does the same thing as the Core fallback
The only in-tree adapter that has listen is XverseAdapter, and its implementation is (e, cb) => listen(e, cb, this.id), which is exactly what line 129 already does. Fordefi and Unisat fall through, and defaultAdapters is private with no setter, so consumers can't plug in another implementation. The branch adds an adapter construction per call, and its tests have to mutate the private field to reach it (tests/listeners.test.mjs:146,166). Consider dropping lines 122-125 and calling listenProvider directly. (It does mirror the adapter-dispatch shape of request/addListener, hence a suggestion only.)
|
|
||
| Xverse suppresses account events while locked without triggering unlock/approval prompts. Connected origins without read permission for the selected account receive `[]`; missing Stacks address/public key also yields `[]`. This does not change existing explicit account requests or legacy events. | ||
|
|
||
| ## Dependency rollout |
There was a problem hiding this comment.
[P3] PR-process notes committed as permanent docs
The "Dependency rollout" and "Validation" sections and the "companion to sats-connect-core#131" opener describe this PR rather than the library, and they go stale as soon as Core 0.19.0 ships. The README links to this file as the usage doc. Consider moving those sections to the PR description and keeping only the usage, provider-selection and Gaia-policy content.
| assert.deepEqual(calls, ['stx_networkChange', 'stx_accountChange', 'stx_networkChange']); | ||
| }); | ||
|
|
||
| test('delegates to an adapter on one instance and retains its native receiver', () => { |
There was a problem hiding this comment.
[P3] Tests cover an unreachable adapter configuration and miss the real non-Xverse adapters
The adapter-delegation and withAdapter tests inject custom defaultAdapters, which production code can't do. No test calls Wallet.listen with a real defaultAdapters entry that lacks listen (for example unisat or a Fordefi provider), and that is the path most non-Xverse users take. Consider replacing the private-field cases with a setDefaultProvider('unisat') case that asserts the chosen failure behaviour.
|
[agent] Decisions this PR makes that I reviewed and agree with, at New entry point
Assumption
Failure handling
Consistency
|
|
[agent] Update to the decisions I reviewed, at Now agreed Consistency
|
Package pre-release info
Built at (UTC): 2026-10-03T18:16:12.120Z
Summary
Companion to Core/SDK #131, extension #2265, mobile #3007, and Stacks Connect #515.
Xverse's injected provider exposes SIP-030
listendirectly: neither Stacks Connect nor Sats Connect is required to call the native wallet API. This PR closes the top-level Sats Connect integration gap.@sats-connect/core@0.19.0-d1718beprerelease in the manifest and lockfile. This exposes typedstx_getNetworks, the two SIP event contracts, and the namedlistenexport through the existing Core re-export.Wallet.listen(event, callback)forstx_networkChangeandstx_accountChangeto the default Wallet API. Honor the instance's selected provider (or adopt its saved default) without selection, approval or unlock prompts.listen; otherwise use the injected provider's native listener through Core. Preserve callback receiver, bare native payloads and the exact unlisten function.stx_getAccountsautomatically.Wallet.addListener, positional/object calling conventions, and RPC response payloads unchanged.https://gaia.invalid, for software and hardware accounts. Sats Connect does not replace another wallet's Gaia fields.Validation
npm ci --ignore-scripts --no-audit --no-fundagainst published dependencies.npm run check-types: source and positive/negative API type fixtures.npm run test:listeners: build/type declarations and 9 passing public-artifact tests.git diff --check.Tests cover saved/instance provider routing, both event payloads, independent cleanup and receiver forwarding, adapters and native fallback, unsupported providers, no implicit UI/RPC calls, named exports and discovery, and unchanged legacy callbacks.
Rollout
Draft pending stable dependency rollout: temporarily use SDK
0.19.0-d1718be, then replace its manifest/lockfile pin with stable0.19.0once available before a production release. No local tarball paths or guessed published versions.Keep the already-planned, unpublished
sats-connectrelease version4.3.0; no existing stable version is overwritten.