Skip to content

feat: update sendTransfer params - #2634

Merged
edgar-stacks merged 8 commits into
devfrom
feat/sendTransfer-params
Aug 27, 2026
Merged

edgar-stacks merged 8 commits into
devfrom
feat/sendTransfer-params

Conversation

@edgar-stacks

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actions Bot commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

Leather Web build 43826cb — Web staging

@github-actions

github-actions Bot commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

Leather Extension build 43826cb — Extension build

@claude

claude Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Claude finished @edgar-stacks's task in 2m 22s —— View job


PR Review

Overall the change looks solid and focused. A few items worth flagging:

Bugs / correctness

  • Multisig proposal returns an unsigned tx under transaction — apps/extension/src/app/pages/rpc-send-transfer/use-rpc-send-transfer-actions.tsx:132 returns bytesToHex(getPsbtAsTransaction(base64.decode(rawPayload)).unsignedTx) under the transaction field. In the broadcast/sign-only branches, transaction is the finalized/signed hex; in the multisig branch it is the unsigned tx. The sendTransfer schema doc-comment in packages/rpc/src/methods/bitcoin/send-transfer.ts:37 only explains proposalId/status for the multisig case — a dApp reading transaction will get semantically different content depending on the branch. Either (a) don't return transaction in the proposed branch, or (b) update the schema doc-comment to explicitly state that in the status: 'proposed' case transaction is the unsigned PSBT/tx hex.

  • Taproot warning moved before signing (behavior change) — use-rpc-send-transfer-actions.tsx:146-147 now runs checkIfInputsIncludeTaproot before signTransaction, and broadcastTx is called with skipTaprootWarning: true. This is required for the sign-only branch to gate on the warning, but it's a behavior change for the existing broadcast path (user now sees the warning before signing rather than after). The E2E test only covers the sign-only cancel path — worth also asserting the taproot warning still appears in the normal broadcast path. Not a bug per se, just a broader-scope behavior change than the PR title implies.

  • isBroadcasting state naming — in the sign-only branch, isBroadcasting gates the "Signing..." busy label via busyLabel. It works, but the state name is now misleading (we're signing, not broadcasting). Optional rename to something like isSubmitting. Nit.

Security

  • The origin string is rendered into the NoBroadcastWarningLabel copy. React escapes text nodes by default, so no XSS risk — noting for completeness.
  • broadcast param round-trips through URL search params as a string. Round-trip logic is safe: writer serializes with String(params.broadcast ?? true); reader treats anything other than the exact string 'false' as true (initialSearchParams.get('broadcast') !== 'false'). Fail-closed on ambiguous values (default to broadcasting when unclear) is the safer default here — good.

CLAUDE.md conventions

  • Clean — no as/!/any, no enums, kebab-case filenames, function declarations at top level, no throws in render paths. Extension component doesn't use lingui (consistent with rest of extension); mobile does. Good.

Nits (ignore if you like)

  • Extension warning ends without a period: Continue only if you trust {origin} (no-broadcast-warning-label.tsx:12). Mobile variant has inconsistent trailing period between the origin/no-origin branches (psbt-request-no-broadcast-warning-label.tsx:14-15).
  • E2E test hardcodes noBroadcastWarningTitle as a local string literal — if the component copy changes, the test won't auto-fail on the mismatch. Consider importing the string from the component.

Nothing blocking.
• Branch: feat/sendTransfer-params

@codecov

codecov Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 12.84404% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.51%. Comparing base (a754f95) to head (43826cb).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
...pc-send-transfer/use-rpc-send-transfer-actions.tsx 0.00% 38 Missing ⚠️
...res/approver/components/approver-buttons/index.tsx 0.00% 13 Missing ⚠️
.../features/browser/approver-sheet/send-transfer.tsx 0.00% 11 Missing ⚠️
...onents/psbt-request-no-broadcast-warning-label.tsx 0.00% 11 Missing ⚠️
...ps/mobile/src/features/psbt-signer/psbt-signer.tsx 0.00% 11 Missing ⚠️
.../app/pages/rpc-send-transfer/rpc-send-transfer.tsx 0.00% 6 Missing ⚠️
...p/pages/rpc-send-transfer/use-rpc-send-transfer.ts 0.00% 3 Missing ⚠️
...pc-transaction-request/get-transaction-actions.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2634      +/-   ##
==========================================
- Coverage   19.51%   19.51%   -0.01%     
==========================================
  Files        2471     2474       +3     
  Lines      111410   111514     +104     
  Branches     6326     6330       +4     
==========================================
+ Hits        21746    21766      +20     
- Misses      88759    88843      +84     
  Partials      905      905              
Files with missing lines Coverage Δ
...transaction-request/no-broadcast-warning-label.tsx 100.00% <100.00%> (ø)
...ges/rpc-send-transfer/rpc-send-transfer.context.ts 0.00% <ø> (ø)
.../background/messaging/rpc-methods/send-transfer.ts 76.78% <100.00%> (+0.42%) ⬆️
.../extension/src/shared/rpc/methods/send-transfer.ts 95.12% <100.00%> (+0.18%) ⬆️
...pc-transaction-request/get-transaction-actions.tsx 0.00% <0.00%> (ø)
...p/pages/rpc-send-transfer/use-rpc-send-transfer.ts 0.00% <0.00%> (ø)
.../app/pages/rpc-send-transfer/rpc-send-transfer.tsx 0.00% <0.00%> (ø)
.../features/browser/approver-sheet/send-transfer.tsx 0.00% <0.00%> (ø)
...onents/psbt-request-no-broadcast-warning-label.tsx 0.00% <0.00%> (ø)
...ps/mobile/src/features/psbt-signer/psbt-signer.tsx 0.00% <0.00%> (ø)
... and 2 more
Components Coverage Δ
bitcoin 84.90% <ø> (ø)
query 26.74% <ø> (ø)
utils 89.04% <ø> (ø)
crypto 78.38% <ø> (ø)
stacks 65.32% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@heidar-stacks
heidar-stacks requested review from jannik-stacks and a lite review from Copilot August 20, 2026 15:22

Copilot AI left a comment

Copy link
Copy Markdown

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 extends the Bitcoin sendTransfer RPC method to support an optional broadcast parameter (allowing “sign only” behavior) and to return the signed raw transaction hex (transaction) in responses across mobile + extension flows.

Changes:

  • Add optional broadcast to both legacy and new sendTransfer param schemas and propagate it through extension request plumbing.
  • Extend sendTransfer result shape to optionally include transaction and update mobile/extension implementations to return it (and to skip broadcasting when broadcast: false).
  • Add/extend unit + E2E coverage for broadcast handling and the updated result schema.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/rpc/src/methods/bitcoin/send-transfer.ts Adds broadcast param support and extends result schema with optional transaction.
packages/rpc/src/methods/bitcoin/send-transfer.spec.ts New schema-focused unit tests for broadcast params and result acceptance.
apps/mobile/src/features/browser/approver-sheet/send-transfer.tsx Passes broadcast through to the signer (defaulting to true) and returns transaction in the RPC result.
apps/extension/tests/specs/rpc-send-transfer/rpc-send-transfer.spec.ts Updates E2E expectations to include transaction and adds a no-broadcast test verifying no broadcast call occurs.
apps/extension/src/shared/rpc/methods/send-transfer.ts Adds broadcast to extension-side zod schemas and carries it through legacy→new param conversion.
apps/extension/src/shared/rpc/methods/send-transfer.spec.ts Adds schema/legacy-conversion tests covering broadcast.
apps/extension/src/background/messaging/rpc-methods/send-transfer.ts Propagates broadcast into popup URL params (defaulting to true).
apps/extension/src/app/pages/rpc-send-transfer/use-rpc-send-transfer.ts Parses broadcast from URL params into request state.
apps/extension/src/app/pages/rpc-send-transfer/use-rpc-send-transfer-actions.tsx Implements “sign only” flow when broadcast is false; returns transaction in responses (including multisig proposal flow).
apps/extension/src/app/pages/rpc-send-transfer/rpc-send-transfer.context.ts Extends RPC send-transfer context to include broadcast.
Suppressed comments (1)

packages/rpc/src/methods/bitcoin/send-transfer.ts:43

  • The sendTransfer result schema is currently fully-optional, so an empty {} result would validate successfully. This makes it easy for implementations to accidentally return an invalid/meaningless response (e.g., missing txid, transaction, and multisig fields) without being caught by schema validation.
  result: z.object({
    txid: z.string().optional(),
    transaction: z.string().optional(),
    // Present (and `status: 'proposed'`) when the active account is a multisig
    // policy: the transaction was proposed to the coordinator, not broadcast, so
    // `txid` is empty until co-signers complete and broadcast it.
    proposalId: z.string().optional(),
    status: z.enum(['broadcast', 'proposed']).optional(),
  }),

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@edgar-stacks
edgar-stacks merged commit 344da46 into dev Aug 27, 2026
45 checks passed

This branch was previously deployed

1 inactive deployment
web:staging — 43826cb4 Deployed Aug 26, 2026 by edgar-stacks via staging #968
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.

add "txHex" response param and "broadcast" to sendTransfer rpc request

5 participants