refactor: use specific address type in ambiguity check (#1404)

src/lib/formValidation.test.ts

@@ -1,4 +1,4 @@
-import { Network } from 'bitcoin-address-validation'
+import { getAddressInfo, Network } from 'bitcoin-address-validation'
 import { describe, expect, it } from 'vitest'
 import type { AddressSummary } from '@/context/JamWalletInfoContext'
 import {
@@ -16,18 +16,30 @@ const testnetBech32Address = 'tb1qw508d6qejxtdg4y5r3zarvary0c5xw7kxpjzsx'
 // bech32m/taproot pair - same witness program, differing only by HRP (tb1p vs bcrt1p)
 const testnetTaprootAddress = 'tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq47zagq'
 const regtestTaprootAddress = 'bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqc8gma6'
-const regtestLegacyAddressLabeledTestnet = 'mkpZhYtJu2r87Js3pDiWJDmPte2NRZ8bJV'
+const regtestLegacyAddressLabelledTestnet = 'mkpZhYtJu2r87Js3pDiWJDmPte2NRZ8bJV'
 
 const addressSummary = {
   [mainnetAddress]: { address: mainnetAddress, used: false },
 } as unknown as AddressSummary
 
 describe('isValidAddress', () => {
-  it('accepts a valid address and rejects everything else', () => {
-    expect(isValidAddress(mainnetAddress)).toBe(true)
+  it.each([
+    mainnetAddress,
+    testnetAddress,
+    regtestBech32Address,
+    testnetBech32Address,
+    testnetTaprootAddress,
+    regtestTaprootAddress,
+    regtestLegacyAddressLabelledTestnet,
+  ])('accepts valid addresses', (address) => {
+    expect(isValidAddress(address)).toBe(true)
+  })
+
+  it('reject invalid addresses', () => {
     expect(isValidAddress('not-an-address')).toBe(false)
     expect(isValidAddress('')).toBe(false)
     expect(isValidAddress(undefined)).toBe(false)
+    expect(isValidAddress(null)).toBe(false)
     expect(isValidAddress(42)).toBe(false)
   })
 })
@@ -36,6 +48,7 @@ describe('isAddressOnNetwork', () => {
   it('matches the address network', () => {
     expect(isAddressOnNetwork(mainnetAddress, Network.mainnet)).toBe(true)
     expect(isAddressOnNetwork(mainnetAddress, Network.testnet)).toBe(false)
+    expect(isAddressOnNetwork(testnetAddress, Network.mainnet)).toBe(false)
     expect(isAddressOnNetwork(testnetAddress, Network.testnet)).toBe(true)
   })
 
@@ -64,13 +77,12 @@ describe('isAddressOnNetwork', () => {
   })
 
   it('treats testnet and regtest as interchangeable for ambiguous base58 addresses', () => {
+    expect(getAddressInfo(regtestLegacyAddressLabelledTestnet).network, 'sanity check').toBe(Network.testnet)
+
     // A base58 (P2PKH here; P2SH shares the trait) address on a regtest wallet is labeled "testnet"
     // by the library because the two share version bytes, so it must still be accepted on regtest.
-    expect(isAddressOnNetwork(regtestLegacyAddressLabeledTestnet, Network.regtest)).toBe(true)
-    expect(isAddressOnNetwork(regtestLegacyAddressLabeledTestnet, Network.testnet)).toBe(true)
-    // mainnet is never ambiguous with testnet/regtest.
-    expect(isAddressOnNetwork(mainnetAddress, Network.regtest)).toBe(false)
-    expect(isAddressOnNetwork(mainnetAddress, Network.testnet)).toBe(false)
+    expect(isAddressOnNetwork(regtestLegacyAddressLabelledTestnet, Network.regtest)).toBe(true)
+    expect(isAddressOnNetwork(regtestLegacyAddressLabelledTestnet, Network.testnet)).toBe(true)
   })
 })
 

src/lib/formValidation.ts

@@ -1,4 +1,10 @@
-import { getAddressInfo, Network, validate as isValidBitcoinAddress } from 'bitcoin-address-validation'
+import {
+  getAddressInfo,
+  Network,
+  validate as isValidBitcoinAddress,
+  AddressType,
+  type AddressInfo,
+} from 'bitcoin-address-validation'
 import * as yup from 'yup'
 import type { AddressSummary } from '@/context/JamWalletInfoContext'
 import type { BitcoinAddress, BlockHeight, JarIndex } from '@/types/global'
@@ -11,22 +17,32 @@ import { isValidInteger } from './utils'
 export const isValidAddress = (value: unknown): value is BitcoinAddress =>
   typeof value === 'string' && isValidBitcoinAddress(value)
 
-// Base58 addresses (p2pkh/p2sh) share the same version bytes on testnet and regtest, so
-// bitcoin-address-validation can't tell them apart and always labels them "testnet".
+// p2pkh/p2sh addresses (base58) share the same version bytes on testnet and regtest, so
+// address validation can't tell them apart and always labels them "testnet".
 // Treat the two as interchangeable for those so a regtest wallet doesn't reject its own
-// legacy-style addresses as "wrong network". Bech32 addresses are excluded below because
-// their distinct HRP (tb1 vs bcrt1) already identifies the network unambiguously.
-const AMBIGUOUS_TESTNET_REGTEST_NETWORKS: ReadonlySet<Network> = new Set([Network.testnet, Network.regtest])
+// legacy-style addresses as "wrong network".
+// Types like p2wpkh/p2tr (bech32) carry a distinct HRP per network (tb1 vs bcrt1), so they are
+// already identified correctly - don't relax them, or a bcrt1… (regtest) address would
+// wrongly pass on a testnet wallet and vice versa. Only base58 addresses are ambiguous.
+const AMBIGUOUS_NETWORKS_BY_ADDRESS_TYPE: ReadonlyMap<AddressType, Network[]> = new Map([
+  [AddressType.p2pkh, [Network.testnet, Network.regtest]],
+  [AddressType.p2sh, [Network.testnet, Network.regtest]],
+])
 
-export const isAddressOnNetwork = (value: string, network: Network): boolean => {
+const isAmbiguousAddressForNetwork = (info: AddressInfo, expectedNetwork: Network) => {
+  // Bech32/bech32m addresses carry a distinct HRP per network (tb1 vs bcrt1), so they are
+  // already identified correctly - don't relax them, or a bcrt1… (regtest) address would
+  // wrongly pass on a testnet wallet and vice versa. Only base58 addresses are ambiguous.
+  return [info.network, expectedNetwork].every(
+    (it) => AMBIGUOUS_NETWORKS_BY_ADDRESS_TYPE.get(info.type)?.includes(it) ?? false,
+  )
+}
+
+export const isAddressOnNetwork = (value: string, expectedNetwork: Network): boolean => {
   try {
-    const { network: addressNetwork, bech32 } = getAddressInfo(value)
-    if (addressNetwork === network) return true
-    // Bech32/bech32m addresses carry a distinct HRP per network (tb1 vs bcrt1), so they are
-    // already identified correctly - don't relax them, or a bcrt1… (regtest) address would
-    // wrongly pass on a testnet wallet and vice versa. Only base58 addresses are ambiguous.
-    if (bech32) return false
-    return AMBIGUOUS_TESTNET_REGTEST_NETWORKS.has(addressNetwork) && AMBIGUOUS_TESTNET_REGTEST_NETWORKS.has(network)
+    const addressInfo = getAddressInfo(value)
+    if (addressInfo.network === expectedNetwork) return true
+    return isAmbiguousAddressForNetwork(addressInfo, expectedNetwork)
   } catch (_ignoredOnPurpose) {
     return false
   }