Skip to content

feat(rn_cli_wallet): add Bitcoin (bip122) support#565

Open
ignaciosantise wants to merge 4 commits into
mainfrom
feat/bitcoin-support-rn-cli-wallet
Open

feat(rn_cli_wallet): add Bitcoin (bip122) support#565
ignaciosantise wants to merge 4 commits into
mainfrom
feat/bitcoin-support-rn-cli-wallet

Conversation

@ignaciosantise

Copy link
Copy Markdown
Collaborator

Summary

Adds Bitcoin (bip122) support to wallets/rn_cli_wallet, ported from the web react-wallet-v2 sample and wired into the existing per-chain conventions (constants + lib + wallet/handler utils + modals), plus supporting UI/balance changes.

What's included

Bitcoin core (bip122, mainnet)

  • BitcoinLib with HD derivation for both P2WPKH (payment) and P2TR (ordinals/taproot) addresses, and the 4 methods: signMessage, getAccountAddresses, sendTransfer, signPsbt (UTXO/fee/broadcast via mempool.space).
  • New constants/Bitcoin.ts, BitcoinWalletUtil, BitcoinRequestHandlerUtil, and 3 signing modals.
  • Wired into session approval (SessionProposalModal), request routing (useWalletKitEventsManager), ModalStore, Modal.tsx, SettingsStore, the init hook and PresetsUtil.

Crypto notes (RN/Hermes)

  • Uses pure-JS @bitcoinerlab/secp256k1 instead of the web sample's tiny-secp256k1 (WASM, won't run under Hermes); bitcoinjs-lib/bip32/ecpair accept it via their factories.
  • Wires @noble/secp256k1 v3 sync hashes from @noble/hashes (its schnorr sync API otherwise throws "hashes.sha256 not set").
  • Uses the global (react-native-compat) Buffer so types match bitcoinjs-lib.

Import wallet UI

  • Replaces the cramped 6-item SegmentedControl with a scalable NetworkDropdown (icon + name + inline scrollable list) and adds Bitcoin as an import option.
  • Dropdown closes when the input is focused.

Secret keys screen

  • Shows the Bitcoin recovery phrase.

Balances

  • Requests bip122 from the balance API and always shows a Bitcoin row.
  • Distinguishes a confirmed 0 from an unknown balance: when a chain's balance request fails/unsupported (the API 400s on bip122/sui), the native row renders ~ instead of 0.
  • Downgrades expected per-chain 400s from error to warn (the service already fails silently per-chain).

Misc

  • Suppresses the "Invalid bundle ID detected" warning on web only (expo-application has no bundle ID there); native still warns.

Verification

  • tsc and eslint clean (only a pre-existing DesktopFrameWrapper.web.tsx window error remains, untouched).
  • Node smoke test of the crypto stack (derivation of bc1q…/bc1p…, ECDSA + schnorr signing, PSBT build) passes.
  • expo export --platform ios succeeds (Hermes .hbc bundle, no WASM/resolution errors).

Not yet done (manual)

  • On-device run + a live bip122 dApp session (approve + exercise each method). sendTransfer/signPsbt broadcast are mainnet-only (real BTC) — use signPsbt with broadcast:false as the safe tx-path check.
  • Note: the WalletConnect balance API currently 400s on bip122 (same as sui), so BTC shows ~ until/unless the API adds support.

🤖 Generated with Claude Code

Port Bitcoin (bip122) support from the web react-wallet-v2 sample into the
React Native wallet, following the existing per-chain conventions.

- BitcoinLib + constants/utils/handlers for signMessage, getAccountAddresses,
  sendTransfer and signPsbt (mainnet, P2WPKH + P2TR/ordinals). Uses pure-JS
  @bitcoinerlab/secp256k1 (tiny-secp256k1 is WASM and won't run under Hermes)
  and wires @noble/secp256k1 v3 sync hashes from @noble/hashes.
- Wire bip122 into session approval, request routing, 3 signing modals,
  SettingsStore, init hook and PresetsUtil.
- Import wallet: replace the cramped 6-item SegmentedControl with a scalable
  NetworkDropdown and add Bitcoin as an import option; close the dropdown when
  the input is focused.
- Secret keys screen shows the Bitcoin recovery phrase.
- Balances: request bip122 from the balance API and always show a Bitcoin row.
  Distinguish a confirmed 0 from an unknown balance (failed/unsupported chain)
  by rendering "~" instead of "0"; downgrade expected 400s from error to warn.
- misc: don't warn about a missing bundle ID on web (expo-application has none).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
deposit-app-demo Error Error Jul 8, 2026 7:26pm
pos-demo Ready Ready Preview, Comment Jul 8, 2026 7:26pm

Request Review

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @ignaciosantise's task in 5m 42s —— View job


PR Review: feat(rn_cli_wallet): add Bitcoin (bip122) support

  • Gather context
  • Read key changed files
  • Run license compliance check
  • Run breaking changes check
  • Run data classification check
  • Compile review findings

🚨 PR Too Large Files: 26 Lines: ~2009 Severity: HIGH Category: maintainability
This PR bundles too many independent concerns. Suggested splits: PR 1: Bitcoin crypto core (BitcoinLib, BitcoinWalletUtil, BitcoinRequestHandlerUtil, constants/Bitcoin.ts, new modals) — PR 2: UI changes (NetworkDropdown, ImportWalletModal, SecretPhrase) — PR 3: Balance/store wiring (WalletStore, BalanceService, SettingsStore, ModalStore, PresetsUtil).

🔒 External Domain URLs (Non-blocking)

  • https://mempool.space/apiBitcoinLib.ts:290, BitcoinLib.ts:295, BitcoinLib.ts:313, constants/Bitcoin.ts:33 — Verify intentional; if this endpoint is unreachable or returns unexpected data, sendTransfer/signPsbt will fail or use a wrong fee rate. Consider making the base URL configurable.

Found 7 issue(s)

Issue 1: getPrivateKey() returns the master mnemonic, not a single private key

ID: dcl-bitcoinlib-getprivatekey-mnemonic-b2c1
File: wallets/rn_cli_wallet/src/lib/BitcoinLib.ts:142
Severity: CRITICAL
Category: security

Context:

  • Pattern: getPrivateKey() returns this.mnemonic — the full BIP39 seed phrase that derives every key in the wallet, not a single address private key.
  • Risk: The misleading name causes callers to treat it with the lower sensitivity of a single key, while actually receiving the master secret. Any future consumer that logs, transmits, or caches getPrivateKey() output exposes every derived address.
  • Impact: Complete wallet compromise; all 20 payment and 20 ordinal addresses recoverable by any recipient of the return value.
  • Trigger: Any code path that calls getPrivateKey() under the assumption the result is a disposable single-key value.

Recommendation: Delete getPrivateKey() entirely — getMnemonic() (line 138) already exists and correctly names what it returns.


Issue 2: Mnemonic persisted in plaintext MMKV (no encryption key)

ID: dcl-bitcoinwalletutil-mnemonic-unencrypted-3a7f
File: wallets/rn_cli_wallet/src/utils/BitcoinWalletUtil.ts:33
Severity: CRITICAL
Category: security

Context:

  • Pattern: storage.setItem(BITCOIN_MNEMONIC_KEY, wallet1.getMnemonic()) writes to an MMKV instance constructed without an encryptionKey. MMKV without encryption stores data as cleartext files on the device filesystem.
  • Risk: The code's own comment (// Don't store private keys in local storage in a production project!) acknowledges the problem but provides no mitigation. The DEV-only console.warn (line 74) gives no runtime protection.
  • Impact: Any process, ADB backup, or rooted-device filesystem read recovers the mnemonic, granting full control of all derived Bitcoin keys.
  • Trigger: Physical access, ADB backup (adb backup), or rooted device — all three are realistic on a device used for crypto testing.

Recommendation: Store the mnemonic via react-native-keychain (Keychain/Keystore) instead of MMKV, or at minimum pass encryptionKey sourced from the secure enclave to the MMKV constructor:

const secureStorage = new MMKV({ encryptionKey: await getKeyFromKeychain() });

Issue 3: Wrong BIP derivation path for P2TR (taproot/ordinals) addresses

ID: bitcoinlib-taproot-derivation-path-7c3e
File: wallets/rn_cli_wallet/src/lib/BitcoinLib.ts:343
Severity: HIGH
Category: correctness

Context:

  • Pattern: generateAddress uses m/84'/${coinType}'/0'/... for both P2WPKH and P2TR outputs — the taproot flag only changes the bitcoin.payments.* call, not the derivation path.
  • Risk: P2TR addresses should follow BIP86 (m/86'/0'/0'/...), not BIP84 (m/84'/0'/0'/...). Wallets and signers that comply with BIP86 will derive completely different keys at the same index. An ordinals dApp sending a signPsbt request with BIP86-derived input addresses will fail to match any key held by this wallet.
  • Impact: Taproot/ordinals signing will silently produce wrong addresses; cross-wallet interop breaks; PSBTs referencing BIP86 addresses cannot be signed.
  • Trigger: Any getAccountAddresses call with intentions: ['ordinal'], or any PSBT that references a taproot address generated by a BIP86-compliant external wallet.

Recommendation:

const path = taproot
  ? `m/86'/${coinType}'/0'/${change ? 1 : 0}/${index}`
  : `m/84'/${coinType}'/0'/${change ? 1 : 0}/${index}`;

Issue 4: forEach return doesn't break UTXO iteration — all UTXOs always spent

ID: bitcoinlib-utxo-foreach-no-break-2f1a
File: wallets/rn_cli_wallet/src/lib/BitcoinLib.ts:221
Severity: HIGH
Category: correctness

Context:

  • Pattern:
    utxos.forEach(utxo => {
      utxosValue += utxo.value;
      utxosToSpend.push(utxo);
      if (utxosValue >= satoshis) {
        return; // ← `return` in forEach callback = skip to next, not stop
      }
    });
  • Risk: return inside a forEach callback does not break the iteration; it just skips to the next iteration (equivalent to continue). Every UTXO in the wallet is always added to utxosToSpend regardless of whether the target amount is met.
  • Impact: Consolidates all UTXOs into every transaction, inflating transaction size and fees, degrading on-chain privacy, and potentially causing dust accumulation issues.
  • Trigger: Any sendTransfer call where the wallet has more UTXOs than needed to cover the amount.

Recommendation: Use a for...of loop with an explicit break:

for (const utxo of utxos) {
  utxosValue += utxo.value;
  utxosToSpend.push(utxo);
  if (utxosValue >= satoshis) break;
}

Issue 5: validateSignaturesOfInput(0, validator) hardcodes input index 0 in signPsbt

ID: bitcoinlib-signpsbt-validate-hardcoded-index-9b4d
File: wallets/rn_cli_wallet/src/lib/BitcoinLib.ts:267
Severity: HIGH
Category: correctness

Context:

  • Pattern: After signing all requested inputs, validation is run only on index 0 regardless of which inputs were actually signed.
    transaction.validateSignaturesOfInput(0, validator);
  • Risk: If signInputs doesn't include an input at index 0 (e.g., the dApp only asks to sign inputs 1 and 2), bitcoinjs-lib will throw because input 0 has no signature to validate. Conversely, if index 0 was signed but another signed index has an invalid signature, it goes unchecked.
  • Impact: Crashes on valid multi-input PSBTs where the caller doesn't own index 0; silently passes invalid signatures on inputs > 0.
  • Trigger: Any PSBT where signInputs[0].index !== 0.

Recommendation: Validate each signed input individually:

signInputs.forEach(({ index }) => {
  transaction.validateSignaturesOfInput(index, validator);
});

Issue 6: Bitcoin import doesn't refresh balances — UI stays stale

ID: importwalletmodal-bitcoin-no-fetchbalances-5e2c
File: wallets/rn_cli_wallet/src/modals/ImportWalletModal.tsx:178
Severity: MEDIUM
Category: correctness

Context:

  • Pattern: Every other chain case (Ethereum, Ton, Tron, Sui, Solana) calls WalletStore.fetchBalances(...) after a successful import. The Bitcoin case does not.
  • Risk: After importing a Bitcoin wallet, the Wallets screen still shows the old balance (or ~ from initialization) until the user manually refreshes or restarts.
  • Impact: Confusing UX — the newly imported address appears to have no balance even when it does.

Recommendation:

case 'Bitcoin': {
  const result = await loadBitcoinWallet(sanitizedInput);
  address = result.address;
  WalletStore.fetchBalances(
    {
      eip155Address: SettingsStore.state.eip155Address,
      tonAddress: SettingsStore.state.tonAddress,
      tronAddress: SettingsStore.state.tronAddress,
      suiAddress: SettingsStore.state.suiAddress,
      solanaAddress: SettingsStore.state.solanaAddress,
      bitcoinAddress: address,
    },
    { force: true },
  );
  break;
}

Fix this →


Issue 7: projectId included verbatim in logged URL despite earlier redaction

ID: dcl-balanceservice-projectid-in-logs-d4e2
File: wallets/rn_cli_wallet/src/services/BalanceService.ts:51
Severity: MEDIUM
Category: security

Context:

  • Pattern: Line 30 deliberately redacts the project ID (projectId: projectId ? '***' : 'MISSING'), but line 51 logs the full constructed URL which includes ?projectId=<actual-value> as a query parameter. LogStore persists up to 100 entries to MMKV.
  • Risk: The explicit redaction on line 30 creates false confidence; the credential is visible 21 lines later in the same function.
  • Impact: WalletConnect project ID leaked to device MMKV log storage — recoverable from any device backup or forensic read.

Recommendation: Log a redacted URL copy:

const logUrl = new URL(url.toString());
logUrl.searchParams.set('projectId', '***');
LogStore.log('Fetching URL', 'BalanceService', 'fetchBalanceForChain', {
  url: logUrl.toString(),
});

License compliance: All 6 new dependencies (@bitcoinerlab/secp256k1, @noble/secp256k1, bip32, bitcoinjs-lib, bitcoinjs-message, ecpair) are MIT-licensed. No issues.

Breaking changes: All store, type, and API surface changes are purely additive. No issues.

- Remove misleading getPrivateKey() (dead code; returned the master mnemonic).
- sendTransfer: select UTXOs with for...of + break instead of forEach+return,
  which never stopped and spent every UTXO.
- signPsbt: validate each signed input instead of hardcoding index 0 (would
  throw when the dApp signs inputs not including index 0).
- ImportWalletModal: refetch balances after a Bitcoin import, matching the
  other chains.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ignaciosantise

Copy link
Copy Markdown
Collaborator Author

Thanks for the review 🙏 Actions taken in b49e80dd:

✅ Fixed

  • feat: init rn native wallet #1getPrivateKey() returns the master mnemonic (CRITICAL): removed the method entirely. It was dead code; getMnemonic() already exists and names what it returns.
  • Expo Wallet #4forEach return doesn't break UTXO loop (HIGH): switched to for…of + break, so selection stops once the target amount is covered instead of spending every UTXO.
  • Add example using react-native <0.70.x (pre-Hermes engine) #5validateSignaturesOfInput(0) hardcoded in signPsbt (HIGH): now validates each signed input, so PSBTs where the signed inputs don't include index 0 no longer throw.
  • Issues trying to run the example #6 — Bitcoin import doesn't refresh balances (MEDIUM): the Bitcoin case now calls WalletStore.fetchBalances(..., { force: true }), matching the other chains.

⏸️ Deferred / won't fix (with reasons)

  • Feat/expo wallet #3 — P2TR uses BIP84 path instead of BIP86 (HIGH): valid standards point, but this is a faithful port of the upstream web-examples Bip122Lib, which we keep in sync. The wallet is self-consistent (it signs exactly the addresses it reports). Fixing only here would diverge from upstream and change the wallet's addresses — we'll align this in the web sample first so both stay consistent.
  • Feat: Expo Example #2 — Mnemonic in plaintext MMKV (CRITICAL): this is the existing convention for every chain in this sample (EIP155/Solana/Ton/…), which is a dev/demo wallet with the same "not for production" caveat. Secure storage (Keychain/Keystore) is a repo-wide follow-up, not scoped to Bitcoin.
  • Feat: Add React Native Wallet 0.68.5 #7projectId in logged URL (MEDIUM): pre-existing in BalanceService (this PR only changed log levels there, not URL logging), and unrelated to Bitcoin. Worth a separate cleanup.
  • mempool.space external URL (non-blocking): intentional — it's how bip122 UTXO fetch/broadcast/fee estimation work, ported from the web sample. Making the base URL configurable is a possible nice-to-have.
  • "PR too large": it's one cohesive feature (Bitcoin end-to-end) and a draft; keeping it as a single unit for now.

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

Adds Bitcoin (bip122) support to the wallets/rn_cli_wallet sample wallet, including key derivation/signing utilities, WalletConnect request handling + modals, import/secret phrase UI updates, and balance display logic for unsupported chains.

Changes:

  • Introduces a Bitcoin stack (BitcoinLib, constants, wallet + request handler utils) and wires it into initialization + WalletConnect routing and modals.
  • Updates import wallet and secret phrase screens to support Bitcoin mnemonic handling.
  • Extends balances fetching/rendering to include bip122 and to represent “unknown” balances as ~ when the API rejects a chain.

Reviewed changes

Copilot reviewed 24 out of 26 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
wallets/rn_cli_wallet/yarn.lock Adds locked dependencies for Bitcoin support and related crypto libraries.
wallets/rn_cli_wallet/src/utils/PresetsUtil.ts Adds Bitcoin network images and chains into the presets registry.
wallets/rn_cli_wallet/src/utils/misc.ts Suppresses “Invalid bundle ID” warning on web only.
wallets/rn_cli_wallet/src/utils/BitcoinWalletUtil.ts Adds Bitcoin wallet creation/restore + import flow and persists mnemonic.
wallets/rn_cli_wallet/src/utils/BitcoinRequestHandlerUtil.ts Adds WalletConnect request approve/reject handler for bip122 methods.
wallets/rn_cli_wallet/src/utils/BalanceTypes.ts Adds balanceUnavailable flag to represent unknown balances.
wallets/rn_cli_wallet/src/store/WalletStore.ts Adds Bitcoin address + chain support and “unknown balance” synthesis logic.
wallets/rn_cli_wallet/src/store/SettingsStore.ts Stores Bitcoin address/wallet in app settings state.
wallets/rn_cli_wallet/src/store/ModalStore.ts Adds Bitcoin session modal identifiers.
wallets/rn_cli_wallet/src/services/BalanceService.ts Downgrades expected 400s to warnings and adjusts per-chain failure logging.
wallets/rn_cli_wallet/src/screens/Wallets/index.tsx Adds Bitcoin address mapping and triggers balance fetch when BTC address exists.
wallets/rn_cli_wallet/src/screens/Wallets/components/TokenBalanceCard.tsx Renders ~ SYMBOL when balanceUnavailable is true.
wallets/rn_cli_wallet/src/screens/SecretPhrase/index.tsx Displays the Bitcoin recovery phrase when available.
wallets/rn_cli_wallet/src/modals/SessionProposalModal.tsx Adds bip122 namespace support and exposes both BTC addresses in accounts.
wallets/rn_cli_wallet/src/modals/SessionBitcoinSignMessageModal.tsx New modal to approve/reject Bitcoin message signing.
wallets/rn_cli_wallet/src/modals/SessionBitcoinSendTransactionModal.tsx New modal to approve/reject Bitcoin transfer/PSBT signing.
wallets/rn_cli_wallet/src/modals/SessionBitcoinGetAddressesModal.tsx New modal to approve/reject Bitcoin address sharing.
wallets/rn_cli_wallet/src/modals/ImportWalletModal.tsx Replaces segmented control with a dropdown and adds Bitcoin import.
wallets/rn_cli_wallet/src/lib/BitcoinLib.ts Adds Bitcoin derivation, message signing, transfers, and PSBT signing.
wallets/rn_cli_wallet/src/hooks/useWalletKitEventsManager.ts Routes bip122 signing methods to the new Bitcoin modals.
wallets/rn_cli_wallet/src/hooks/useInitializeWalletKit.ts Initializes/restores Bitcoin wallet on startup and stores it in SettingsStore.
wallets/rn_cli_wallet/src/constants/Bitcoin.ts Adds bip122 constants (chains, methods, events, mainnet CAIP-2 ID).
wallets/rn_cli_wallet/src/components/NetworkDropdown.tsx New dropdown UI component for scalable network selection.
wallets/rn_cli_wallet/src/components/Modal.tsx Registers the new Bitcoin modals in the modal switch.
wallets/rn_cli_wallet/package.json Adds Bitcoin dependencies and adjusts the android script invocation.
Comments suppressed due to low confidence (1)

wallets/rn_cli_wallet/src/store/WalletStore.ts:349

  • fetchBalances returns early when anySuccess is false, which prevents processBalances from synthesizing the BTC (and other native) rows marked as balanceUnavailable. With a Bitcoin-only wallet (or any address group where the balance API 400s), this will likely leave the UI without the intended "~" placeholder row because balances never get updated.
      // Only update state if at least one API call succeeded
      const anySuccess =
        eip155Result.anySuccess ||
        tonResult.anySuccess ||
        tronResult.anySuccess ||
        suiResult.anySuccess ||
        solanaResult.anySuccess ||
        bitcoinResult.anySuccess;

      if (!anySuccess) {
        return;
      }

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

Comment thread wallets/rn_cli_wallet/src/services/BalanceService.ts Outdated
Comment thread wallets/rn_cli_wallet/src/services/BalanceService.ts Outdated
Comment thread wallets/rn_cli_wallet/src/lib/BitcoinLib.ts
Comment thread wallets/rn_cli_wallet/src/modals/SessionBitcoinSignMessageModal.tsx
Comment thread wallets/rn_cli_wallet/src/modals/SessionProposalModal.tsx
- BalanceService: throw a typed BalanceFetchError carrying the HTTP status
  instead of parsing the error message suffix, and stop double-logging expected
  per-chain HTTP failures (already logged in fetchBalanceForChain).
- BitcoinLib.createTransaction: validate all signed inputs, not just index 0.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
P2TR (taproot/ordinals) addresses now derive from m/86' (BIP86) instead of
m/84' (BIP84, which is for P2WPKH). Keeps ordinals addresses interoperable
with other BIP86-compliant wallets. Payment (P2WPKH) addresses stay on m/84'.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ignaciosantise

Copy link
Copy Markdown
Collaborator Author

Update on #3 (taproot derivation path) — decided to fix it here rather than defer to upstream, since it's an unambiguous standards issue. In adf84ab9, P2TR (taproot/ordinals) addresses now derive from BIP86 (m/86'/…) while P2WPKH payment addresses stay on BIP84 (m/84'/…).

Refs:

I'll still flag this to the wallet team so the upstream web-examples Bip122Lib (which has the same m/84' path for taproot) can be aligned.

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