fix: correct three false SDK claims and three real defects from the #23 audit - #25
Merged
Merged
Conversation
… audit The bump PR shipped documentation that teaches code which cannot work, plus three defects no test could see. Nothing crashed, which is why it landed. False claims, each verified against the pinned SDK: - The refusal a pre-flip dApp receives names its version. ConnectClient has sent `sdkVersion` in the handshake since 0.10.1, so a 0.13.1 client is refused with `actualSdk: "0.13.1"`, not `null`. The docs presented the null/"unknown (not reported)" case as what an old dApp sees, and the browser and backend-auth tests were written around that unreachable branch. A reader branching on `actualSdk == null` gets dead code. - "Every pre-0.14 name still fires" is wrong. The flip removed 38 event names and gave 16 an adapter; the other 26 are accepted by `subscribe` and then never emit. 24 of them were live emitters in 0.13.1 — every `invoice:*`, every `swap:*`, `sync:started`/`:error`/`:provider` and more. CONNECT.md now carries the full removed-with-no-adapter table. - `payment_request:accepted` and `:response` were not fabrications: both are declared in 0.13.1's SphereEventType, and `:response` is emitted by PaymentsModule. They were removed by the flip without an adapter. Defects: - badgeFor painted an in-flight `submitted` transfer with the success green, as it did any payload with no `status`. Colour is now driven by the settled set (confirmed/delivered/completed) rather than a blocklist, so an unknown or absent status can no longer answer "did it go through?" with yes. - The bot registered its `inventory:updated` listener after the mint and after the first `assets()` round trip, so the credit event it exists to catch could fire with nobody attached. Subscribe first, mint second, read third — the boot read now goes through the same epoch guard. - mockSphere's paymentsV2 lacked `requests`, which the host dereferences in `sphere.paymentsV2?.requests.list()` — the optional chain guards paymentsV2, not requests, so the first live `payment_request:updated` would throw a TypeError inside ConnectHost. Masked only by `on` being a no-op stub. Also: - sendSafety.test.ts could not detect a code the SDK ADDS, which is the direction that costs money. It now sweeps the whole SphereErrorCode universe through the SDK predicate, with a type-level guard that fails `tsc` and names any code a future SDK adds. - formatAssets moved to bot/src/balance.ts so it is reachable by tests at all; index.ts calls main() at module scope. - nodejs describeConnectFailure and backend-auth describeVersionFloor each handled one of the three handshake-refusal shapes. Both now cover the SDK floor, the protocol floor and the 4008 network mismatch — the last being what a dApp that omits `network` actually hits. The latter is renamed describeHandshakeRefusal to match what it does. - CI runs npm ci instead of npm install. The comment justifying npm install described a file: link that no lockfile carries any more, and npm install will not fail when package.json and package-lock.json disagree.
KruGoL
added a commit
that referenced
this pull request
Aug 8, 2026
* chore(sdk): 0.14.1 -> 0.14.2 across all five packages * fix: correct three false SDK claims and three real defects from the #23 audit (#25) The bump PR shipped documentation that teaches code which cannot work, plus three defects no test could see. Nothing crashed, which is why it landed. False claims, each verified against the pinned SDK: - The refusal a pre-flip dApp receives names its version. ConnectClient has sent `sdkVersion` in the handshake since 0.10.1, so a 0.13.1 client is refused with `actualSdk: "0.13.1"`, not `null`. The docs presented the null/"unknown (not reported)" case as what an old dApp sees, and the browser and backend-auth tests were written around that unreachable branch. A reader branching on `actualSdk == null` gets dead code. - "Every pre-0.14 name still fires" is wrong. The flip removed 38 event names and gave 16 an adapter; the other 26 are accepted by `subscribe` and then never emit. 24 of them were live emitters in 0.13.1 — every `invoice:*`, every `swap:*`, `sync:started`/`:error`/`:provider` and more. CONNECT.md now carries the full removed-with-no-adapter table. - `payment_request:accepted` and `:response` were not fabrications: both are declared in 0.13.1's SphereEventType, and `:response` is emitted by PaymentsModule. They were removed by the flip without an adapter. Defects: - badgeFor painted an in-flight `submitted` transfer with the success green, as it did any payload with no `status`. Colour is now driven by the settled set (confirmed/delivered/completed) rather than a blocklist, so an unknown or absent status can no longer answer "did it go through?" with yes. - The bot registered its `inventory:updated` listener after the mint and after the first `assets()` round trip, so the credit event it exists to catch could fire with nobody attached. Subscribe first, mint second, read third — the boot read now goes through the same epoch guard. - mockSphere's paymentsV2 lacked `requests`, which the host dereferences in `sphere.paymentsV2?.requests.list()` — the optional chain guards paymentsV2, not requests, so the first live `payment_request:updated` would throw a TypeError inside ConnectHost. Masked only by `on` being a no-op stub. Also: - sendSafety.test.ts could not detect a code the SDK ADDS, which is the direction that costs money. It now sweeps the whole SphereErrorCode universe through the SDK predicate, with a type-level guard that fails `tsc` and names any code a future SDK adds. - formatAssets moved to bot/src/balance.ts so it is reachable by tests at all; index.ts calls main() at module scope. - nodejs describeConnectFailure and backend-auth describeVersionFloor each handled one of the three handshake-refusal shapes. Both now cover the SDK floor, the protocol floor and the 4008 network mismatch — the last being what a dApp that omits `network` actually hits. The latter is renamed describeHandshakeRefusal to match what it does. - CI runs npm ci instead of npm install. The comment justifying npm install described a file: link that no lockfile carries any more, and npm install will not fail when package.json and package-lock.json disagree.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to an audit of #23. Stacked on #24 — review that one first; this branch contains
it, so merging this alone is also complete.
Nothing here was crashing. Every test passed, every build succeeded — which is exactly why it
landed. The damage is that the docs teach code which cannot work, and three defects sat in
paths no test exercises.
Every claim below was checked against the pinned SDK, and the handshake ones were reproduced by
driving a real
ConnectHost.False claims
1. The version-floor refusal names the client's version
The docs presented
actualSdk: null/"unknown (not reported)"as what a pre-flip dAppreceives.
ConnectClienthas sentsdkVersionin the handshake since 0.10.1 — only 0.9.xand 0.10.0 report nothing. Driving a real 0.14.1
ConnectHostwithminSdkVersion: '0.14.1-0'(what the Sphere wallet sets) and the exact handshake a 0.13.1 client emits:
A developer branching on
actualSdk == nullwrites a branch that never fires. The claim was inCLAUDE.md,browser/CONNECT.md(including a worked-example JSON no 0.10+ client can receive)and both READMEs, and it had driven
useWalletConnect.test.tsanderrors.test.tsto assert theunreachable case as primary. Both now assert the real refusal, with the null case kept as a
secondary test.
2. "Every pre-0.14 name still fires" — 26 do not
SphereEventTypewent 61 names -> 28.COMPAT_ATTACHERScovers 16 of the 38 removed. The other26 fail silently:
Sphere.on()accepts any string, so the subscribe succeeds and then neverdelivers. 24 of them were live emitters in 0.13.1 —
swap:failedalone had 20 emit sites.Whole families went this way: every
invoice:*, everyswap:*, plussync:started/:error/:provider,inventory:conflict,send:partial-remainder,transfer:invalid,walletapi:session,payment_request:settling.CONNECT.mdnow carries the fullremoved-with-no-adapter table, and a new test keeps a dead name out of the panel's list.
3.
payment_request:accepted/:responsewere not fabricationsA test comment called them names that "never existed in any SDK release". Both are declared in
0.13.1's
SphereEventTypeandSphereEventMap, and:responseis emitted byPaymentsModule.The old list was right; the flip removed them without an adapter.
Defects
badgeForpainted an in-flight transfer green.TransferStatusispending | submitted | confirmed | delivered | completed | failed. The implementation reddenedfailed, amberedpending/deliveryPending, and let everything else fall through to thesuccess colour — so
submitted(certification in flight, money not settled) and any payload withno
statusat all read as "it went through". Colour is now driven by the settled set, so greenmust be earned rather than defaulted into.
The bot could miss the one event it waits for. The
inventory:updatedlistener wasregistered after the mint and after an
await assets()round trip. The server credits a mintasynchronously; if the credit landed in that window the event fired with nobody attached and the
bot printed an empty balance it never revised. Now: subscribe, mint, read — with the boot read
going through the same epoch guard so it cannot outrun an update either.
mockSpherewould throw insideConnectHost.payments-compat.tsrunssphere.paymentsV2?.requests.list()for thepayment_request:paid/:rejected/:expiredadapters. The optional chain guards
paymentsV2, notrequests— and the mock had norequests:Masked only because
mockSphere.onis a no-op, so no event can fire. The facade is nowimplemented whole, with a test that evaluates the exact expression the host does.
Also
sendSafety.test.tscould not detect a code the SDK adds — the direction that costs money,since an unrecognised possibly-committed code falls through to the retryable branch and invites
a double pay. The SDK's
POSSIBLY_COMMITTED_SEND_CODESis module-private, so the test nowsweeps the whole
SphereErrorCodeuniverse through the SDK predicate and compares bothdirections, with a type-level guard for completeness. Verified it bites — removing one code:
formatAssetswas untestable, living in an entry module that callsmain()on import.Moved to
bot/src/balance.tswith unit tests, matching howcoins.ts/sendSafety.ts/aggregatorKey.tsare already structured.nodejs/src/lockResume.tsneverread
err.dataat all despite the README pointing at it as the example of doing so, andbackend-auth's copy dropped the protocol-floor and network branches. Both now cover the SDK
floor, the protocol floor and the 4008 network mismatch — the last being what a dApp that omits
networkactually hits.describeVersionFlooris renameddescribeHandshakeRefusal.npm ci. The comment justifyingnpm installdescribed afile:link no lockfilecarries any more, and
npm installwill not fail whenpackage.jsonandpackage-lock.jsondisagree — precisely the regression an SDK-bump PR must be guarded against.
Verification
npm ciTypechecks were run with the exact commands CI uses (
tsc -bfor browser and backend-authfrontend,
tsc --noEmitelsewhere).