Skip to content

fix: detect WERR_REVIEW_ACTIONS by duck-type, not constructor.name - #44

Merged
sirdeggen merged 2 commits into
bsv-blockchain:masterfrom
BSVanon:fix/werr-review-actions-code-preservation
May 7, 2026
Merged

fix: detect WERR_REVIEW_ACTIONS by duck-type, not constructor.name#44
sirdeggen merged 2 commits into
bsv-blockchain:masterfrom
BSVanon:fix/werr-review-actions-code-preservation

Conversation

@BSVanon

@BSVanon BSVanon commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

The renderer-side handler in src/onWalletReady.ts uses error?.constructor.name === 'WERR_REVIEW_ACTIONS' to detect review-actions errors at three sites (createAction, signAction, internalizeAction). The Vite production build (esbuild minification) renames class identifiers, so at runtime constructor.name returns mangled values like 'a' and the check returns false. Errors then fall through to the generic else branch which only emits {message: ...} — dropping code: 5, reviewActionResults, sendWithResults, txid, tx, and noSendChange from the response body.

The calling app's @bsv/sdk HTTPWalletJSON checks data.code === 5 (and matching siblings 6, 7, 8) to reconstruct the proper WERR_REVIEW_ACTIONS instance with the signed transaction and review reasons. Without code: 5 reaching the wire, the SDK falls through to its own generic Error wrapper, leaving the calling app unable to:

  • Recover the signed transaction (the tx AtomicBEEF) for fallback broadcast through other providers
  • Surface the structured review reason ('serviceError' | 'doubleSpend' | 'invalidTx') to the user
  • Distinguish review-actions failures from generic errors

This makes any flow using acceptDelayedBroadcast: false un-debuggable when the bundled broadcaster encounters a review condition.

How discovered

Hit while integrating BRC-100 wallet support in a DEX (BSV-21 mint flow with ~5KB embedded-icon deploy script). With BSV Desktop v2.2.7 the createAction returned 400 with body {"message":"Undelayed createAction or signAction results require review."} — no code, no tx, no reviewActionResults. With MetaNet Desktop on the same flow + script size, the SDK reconstructed WERR_REVIEW_ACTIONS correctly via case 5: dispatch, surfacing the review reason and providing the signed tx for manual fallback broadcast.

Verified the JSON.stringify produces the expected fields when isolated:

```js
const e = new sdk.WERR_REVIEW_ACTIONS([{txid:'a',status:'serviceError'}], [{txid:'a',status:'sending'}], 'a', [1,2,3], ['a.0']);
console.log(JSON.stringify(e));
// {"reviewActionResults":[...],"sendWithResults":[...],"txid":"a","tx":[1,2,3],"noSendChange":["a.0"],"isError":true,"code":5,"name":"WERR_REVIEW_ACTIONS"}
```

So the serialization itself is fine — the bug is the detection check failing under minification, which routes the error to the wrong branch entirely.

Fix

Replace the brittle constructor.name string check with a duck-type predicate matching on the canonical error message + presence of the reviewActionResults array:

```ts
function isWerrReviewActions(error: unknown): error is { reviewActionResults: unknown[] } {
if (typeof error !== 'object' || error === null) return false;
const e = error as { message?: unknown; reviewActionResults?: unknown };
return (
e.message === 'Undelayed createAction or signAction results require review.' &&
Array.isArray(e.reviewActionResults)
);
}
```

This is robust to:

  • Bundler class-name minification (esbuild, terser, etc.)
  • The cross-package class identity issue: errors are thrown by @bsv/wallet-toolbox's WERR_REVIEW_ACTIONS class but checked here against an import of @bsv/sdk's WERR_REVIEW_ACTIONS class — these are separate class declarations, so instanceof would not work as a replacement either.

The duck-type check fires for both wallet-toolbox-thrown and SDK-thrown errors that genuinely match the WERR_REVIEW_ACTIONS shape, and would not false-positive on unrelated errors (the canonical message string is unique to this error type).

Test plan

  • `npm run build` — renderer + electron build clean
  • `npm test` — translations test (13 tests passing)
  • `npx vitest run --config vitest.config.electron.ts test/onWalletReady.test.ts` — onWalletReady tests (12 tests passing)
  • End-to-end manual test: createAction with `acceptDelayedBroadcast: false` and a deploy script that triggers the bundled broadcaster's review path, verify `data.code === 5` reaches the SDK and `case 5:` dispatch reconstructs `WERR_REVIEW_ACTIONS` with the signed `tx` field

🤖 Generated with Claude Code

The Vite production build (esbuild minification) renames class names,
so `error?.constructor.name === 'WERR_REVIEW_ACTIONS'` returns mangled
identifiers and the check fails at runtime. Errors then fall through
to the generic `{message: ...}` wrapper which strips `code: 5`,
`reviewActionResults`, `sendWithResults`, `txid`, `tx`, and
`noSendChange` from the response body.

The calling app's `@bsv/sdk` HTTPWalletJSON dispatch checks
`data.code === 5` to reconstruct WERR_REVIEW_ACTIONS with the signed
transaction and review reasons. Without `code: 5` it falls through to
its own generic Error wrapper, leaving the app unable to recover the
signed tx or surface review reasons to the user — so flows that use
`acceptDelayedBroadcast: false` and hit any review-actions condition
(serviceError, doubleSpend, invalidTx) become un-debuggable.

Replace the constructor.name check with a duck-type predicate that
matches on the canonical message + presence of the
`reviewActionResults` array. This is robust to bundler minification
and to the cross-package class identity issue (errors thrown by
`@bsv/wallet-toolbox`'s WERR_REVIEW_ACTIONS class are not
`instanceof @bsv/sdk`'s WERR_REVIEW_ACTIONS class — they are
separate class declarations).

Applies to all three handlers: createAction, signAction,
internalizeAction.

Tests: full build clean (renderer + electron), all existing tests
pass (translations + onWalletReady — 25 total).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sirdeggen

Copy link
Copy Markdown
Contributor

Thanks for the contribution. Taking a look

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes renderer-side detection of WERR_REVIEW_ACTIONS errors in src/onWalletReady.ts so that Vite/esbuild minification doesn’t break error classification and strip important structured fields (code, reviewActionResults, tx, etc.) from HTTP responses returned to calling apps.

Changes:

  • Replaced error?.constructor.name === 'WERR_REVIEW_ACTIONS' checks with a new isWerrReviewActions(...) duck-typing predicate.
  • Updated the createAction, signAction, and internalizeAction error branches to use the new predicate before re-wrapping into @bsv/sdk’s WERR_REVIEW_ACTIONS.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/onWalletReady.ts Outdated
Comment thread src/onWalletReady.ts Outdated
@sirdeggen
sirdeggen merged commit 0ee818d into bsv-blockchain:master May 7, 2026
3 checks passed
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.

3 participants