Skip to content

feat(connect): PSBT signing, NFT mint/swap flows, and error callbacks - #5

Merged
TheArtofSatoshi merged 1 commit into
mainfrom
feat/connect-protocol
Jul 26, 2026
Merged

feat(connect): PSBT signing, NFT mint/swap flows, and error callbacks#5
TheArtofSatoshi merged 1 commit into
mainfrom
feat/connect-protocol

Conversation

@cdonnachie

Copy link
Copy Markdown

Summary

Extends the photonic-connect deep-link protocol with five new request types built on top of the existing sign-request handshake, so a dApp (e.g. realm.rxd) can drive real wallet actions, not just message signing:

  • psbt-sign-request — sign a Radiant PSBT. New BIP-174-profile module (packages/lib/src/psbt), byte-compatible with Radiant Core (bare CTxOut under key 0x00, legacy-serialized unsigned tx, FORKID sighash via radiantjs's Sighash.sign). Returns the (possibly partial) signed PSBT, or finalizes/broadcasts and returns a txid when the request opts in with broadcast: true.
  • mint-request — mint an NFT from dApp-supplied metadata + content (embedded or remote), self-funded from the wallet's own UTXOs (never dApp-specified inputs), with a broadcast: false dry-run mode to inspect the built commit/reveal hex before anything is sent.
  • swap-offer-request / swap-accept-request / swap-cancel-request — list, buy, and cancel NFT-for-RXD offers on top of the existing raw-tx-hex "PSRT" swap primitive, including an optional marketplace platform fee (feeRxd/feeAddress) that sits alongside — but distinct from — creator royalty.
  • A generic error callback (#error=<code>&message=...), distinct from both success and explicit user-reject, fired from every request type's failure path so a dApp can tell "wallet-side failure" (locked, insufficient funds, not found, already spent) apart from "user declined" instead of just hanging until its own timeout.

Also:

  • The swap-accept approval screen now resolves and shows the actual NFT being purchased (thumbnail + name), not just the price, by looking up the PSRT's reserved prevout — previously it only ever showed price.
  • signPsbt/finalizePsbt now cryptographically verify an existing partial signature (via Sighash.verify) before trusting it, rather than a pubkey-hash match alone — closes a gap where a malformed/malicious PSBT could get finalized as "complete" without a valid signature.
  • Post-broadcast bookkeeping in the swap-offer and mint flows is now best-effort: a failure after the irreversible on-chain broadcast no longer surfaces as a false "error" to the dApp.

See docs/psbt.md, docs/mint-request.md, and docs/swap-request.md for the full wire-format writeups, including the naming disambiguation between the new BIP-174 PSBT container, the pre-existing "PSRT" swap convention, and the unrelated DeployMethod: "psbt" mint-reveal helper.

Test plan

  • pnpm --filter @photonic/lib test — 942 passed / 22 intentionally-skipped regtest
  • pnpm --filter @photonic/app test — 504 passed
  • tsc --noEmit clean in both packages/lib and packages/app
  • Manual: exercise each new connect request type end-to-end against a running dApp (mint, list, buy, cancel) and confirm callback/error-callback behavior

Extends the photonic-connect deep-link protocol with five new request
types built on top of the existing sign-request handshake:

- psbt-sign-request: sign a Radiant PSBT (new BIP-174-profile module in
  packages/lib/src/psbt, byte-compatible with Radiant Core), returning
  the signed PSBT or broadcasting when requested.
- mint-request: mint an NFT from dApp-supplied metadata/content, funded
  from the wallet's own UTXOs, with a broadcast:false dry-run mode.
- swap-offer-request / swap-accept-request / swap-cancel-request: list,
  buy, and cancel NFT-for-RXD offers built on the existing PSRT swap
  primitive, including an optional marketplace platform fee alongside
  creator royalty.

Also adds a generic error callback (distinct from success and explicit
reject) so a dApp can tell a genuine wallet-side failure (locked,
insufficient funds, not found, already spent) apart from the user
declining, across every request type.

Includes an approval-screen preview for swap-accept-request that
resolves and displays the actual NFT being purchased (not just price)
by looking up the PSRT's reserved prevout, and hardens the PSBT signer
to cryptographically verify existing partial signatures rather than
trusting a pubkey-hash match alone.
@TheArtofSatoshi

Copy link
Copy Markdown

Code review

Verdict: high-quality, security-conscious work — LGTM with a few non-blocking suggestions. I found no exploitable flaw in the trust boundary. Everything below is hardening/UX, not a bug that loses funds.

Verification (run against this PR's head in an isolated worktree)

  • @photonic/lib: 942 passed / 22 skipped — matches the PR description exactly; tsc --noEmit clean.
  • @photonic/app: 493 passed, 1 suite failed to load — the failure is src/predict/odds.test.ts, which imports radiantswap (a link:../../../RadiantSwap sibling dependency absent from an isolated worktree). Same cause for the app tsc errors, all confined to predict/*. Environmental, unrelated to this PR — consistent with the 504 count in a checkout that has the sibling repo.

Security analysis

The trust boundary is handled well:

  • Callback discipline. Callbacks are origin-bound at parse time (cleanCallback requires url.origin === declared origin, strips credentials and fragments), results ride in the URL fragment (never the query, so never in server access logs), and oversized URLs fall back to manual copy/paste rather than truncating.
  • Signing policy. FORKID mandatory, SIGHASH_NONE refused, token-bearing inputs refused outright (prevents co-signing a token burn), and only the wallet's own P2PKH-script inputs get signed. broadcast: true requires the literal boolean.
  • The declared-prevout gap is closed correctly. A PSBT's utxo field is attacker-controlled, but enrichPsbt cross-checks every input against db.txo and hard-blocks on script/value mismatch — and Radiant's FORKID sighash commits to the spent script and value anyway, so a lie only ever yields an unusable signature.
  • Both known radiantjs traps are avoided. Output-script decoding goes through the strict anchored parseP2pkhScript, never Script.toAddress() (which has the input-classification fallback spoof), and Sighash.verify — which throws on malformed input — is wrapped in try/catch inside isValidPartialSig.
  • The partial-signature verification fix is real. Garbage bytes claiming the wallet's own pubkey can no longer be finalized as "complete"; both signPsbt and finalizePsbt verify cryptographically before trusting, and there's a test covering exactly that.
  • Mint self-funding. dApps never specify inputs; content is MIME-allow-listed and SVG passes through the existing sanitizer. A dApp-supplied feeRate is bounded because mintToken builds via buildTx, whose feeCheck caps at ~2.4x min-relay — so the worst a hostile feeRate achieves is modest overpayment, not a drain.
  • Double-broadcast defense. The Unlock.tsx fix (clearing onCloseCallback after use) plus the synchronous approveInFlightRef guard in Connect.tsx is a correct belt-and-suspenders pair.
  • Post-broadcast "best-effort bookkeeping" — not reporting an error to the dApp after an irreversible broadcast already succeeded — is the right call, and it's applied consistently in both the mint and swap-offer flows.

Suggestions (non-blocking, roughly in priority order)

  1. The swap-accept approval screen omits the enforced creator royalty. acceptSwapOffer adds royalty outputs when royalty.enforced, so the taker's real debit is price + royalty + marketplace fee + network fee, but the panel shows only price and fee. This matches existing SwapLoad.tsx behavior, so it isn't a regression — but a dApp-initiated flow raises the bar for showing total cost. previewSwapAccept already resolves the glyph, so extending it to call getTokenRoyalty and render a royalty line would close this.
  2. The mint panel doesn't show a dApp-supplied feeRate override. Bounded by feeCheck as noted, but the approval screen should display something like "Fee rate: N photons/byte (app-requested)" when req.feeRate is present, so a bumped fee isn't silent.
  3. The mint preview renders pre-sanitization SVG bytes. ContentPreview uses raw request.main.data while the minted payload is sanitizeSvgBytes(...) output. There's no script-execution risk in an <img> context, but what the user approves should be byte-identical to what gets minted — render the sanitized bytes.
  4. Misleading blocker on a fully-signed-but-unfinalized PSBT. signableCount excludes own inputs that already carry a valid partial signature, so a PSBT whose wallet inputs are all signed but not yet finalized trips the "None of this transaction's inputs belong to your wallet" blocker. Safe refusal, wrong message — and it blocks a legitimate finalize-only use.
  5. feeAddress is validated charset-only at parse time. An invalid base58check address only fails later inside p2pkhScript (which does throw cleanly, so no silent burn). Validating base58check and network at parse time would fail faster with a better error code.
  6. Minor. The six near-identical build*CallbackUrl functions could collapse into one generic buildCallbackFragmentUrl(callback, params) helper. Also, the error callback forwards raw Error.message strings to the dApp — currently harmless since txids are public, but a note at the throw sites that messages must stay leak-free would guard against regression.

Code quality

Consistently strong. The PSBT module is defensive in the ways that matter: canonical varint enforcement, duplicate-key rejection, trailing-data checks, bigint value plumbing past 2^53, and unknown-field preservation for byte-identical round-trips. Commentary explains why at every non-obvious decision, the error taxonomy is typed rather than string-matched, and test coverage is dense and adversarial (123 protocol tests, fabricated-signature and non-canonical-varint cases, a buildTx-interchangeability proof). Wire-format docs for all three new surfaces are included. classifyConnectError's string matching is self-admittedly approximate, which is fine as long as it stays labeled a hint — as the docstring already does.

Reviewed with Claude Code

@TheArtofSatoshi
TheArtofSatoshi merged commit de2219a into main Jul 26, 2026
1 check failed
@TheArtofSatoshi
TheArtofSatoshi deleted the feat/connect-protocol branch July 26, 2026 15:48
TheArtofSatoshi added a commit that referenced this pull request Jul 27, 2026
…red callback builder

Closes the remaining review items from #5.

Mint preview showed the RAW dApp-supplied bytes while `buildMintPayload`
embedded the sanitized ones, so for an SVG the user approved a rendering of
markup that never reached the chain. The sanitize-or-passthrough decision now
lives once, in `embeddableContentBytes`, which both the payload builder and
the preview call — they cannot drift apart.

`feeAddress` was charset-validated only, so a corrupted address reached
`p2pkhScript` and threw mid-flow, after approval. It is now base58-decoded at
parse (`cleanPayoutAddress`) for a clear early rejection. That check is
deliberately network-agnostic — radiantjs infers the network from the version
byte and transport code has no business knowing which chain the wallet is on —
so the network comparison lives in `assertFeeAddressNetwork`, in swapFlow,
measured against the wallet's own address rather than a signal so the two can
never disagree.

The eight `build*CallbackUrl` functions repeated the same fragment assembly
and size cap; they now share `composeCallbackUrl`. `buildCallbackUrl` keeps
its long-standing UNCAPPED behaviour via an explicit opt-out rather than
silently gaining a cap: a challenge is bounded only by MAX_MESSAGE_LENGTH
(4096) and the nonce is a segment of it, so capping could turn a working sign
callback into a manual fallback. `buildErrorCallbackUrl` also gains the
privacy contract it was missing — it forwards raw error text to a third party,
so throw sites must keep messages free of balances, addresses and UTXO detail.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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