Zcash ledger implementation #1523
Conversation
📝 WalkthroughWalkthroughImplements a Ledger-backed Zcash client with transport-based constructor, async address retrieval, and a transfer flow that halts pending raw-hex provider support. Adds new unit and e2e tests for the Ledger client, minor e2e formatting tweaks, a dev dependency for Ledger HID transport, and a changeset for a patch release. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (12)
.changeset/green-peaches-thank.md (1)
5-6: Make the changeset explicit about scope, limitations, and user impact.The current description is too terse for a release note. Call out that transfers are not production-ready, explain the signing limitation, and note the flat-fee model so downstreams aren’t surprised.
Apply this diff to enrich the changeset:
adding client ledger + +Adds an experimental Ledger-backed Zcash client. + +- Transaction building works with a flat-fee model. +- Ledger signing is currently blocked due to missing raw transaction hex from data providers; transfer attempts will throw a clear error. +- Marked as not production-ready for transfers. Use the keystore client for actual sends until raw hex is available.packages/xchain-zcash/jest.config.e2e.mjs (1)
6-7: Consider increasing the global e2e timeout for Ledger flows.Ledger interactions often exceed 60s, especially with user confirmation. If the Ledger e2e suite doesn’t override timeouts per test, bumping the global timeout reduces flakiness.
Apply this diff if appropriate:
- testTimeout: 60000, + testTimeout: 200000,If you already set a longer timeout within the Ledger e2e file, you can skip this.
packages/xchain-zcash/src/clientLedger.ts (6)
12-22: Type the transport to prevent runtime shape errors and add a guard.Typing as
anyhides issues. A quick runtime guard also helps produce a clearer error than a nested “send is not a function.”- // eslint-disable-next-line @typescript-eslint/no-explicit-any - private transport: any + private transport: Transport private app: AppBtc | undefined @@ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor(params: UtxoClientParams & { transport: any }) { + constructor(params: UtxoClientParams & { transport: Transport }) { super(params) - this.transport = params.transport + if (!params?.transport) throw new Error('Ledger transport is required') + this.transport = params.transport }
33-36: Clarify the sync method error to guide users to the async API.Make the message actionable so callers know which method to use.
- throw Error('Sync method not supported for Ledger') + throw Error('ClientLedger.getAddress() is not supported. Use getAddressAsync(index?, verify?) instead.')
50-50: Prefer nullish coalescing when defaulting numeric indexes.
|| 0works here, but?? 0communicates intent and avoids surprises ifwalletIndexcould be0orNaN.- const fromAddressIndex = params?.walletIndex || 0 + const fromAddressIndex = params?.walletIndex ?? 0
56-61: Don’t forcefeeRate: 0; letprepareTxchoose or pass an explicit Zcash fee if supported.Hard-coding 0 may trip internal fee bound checks. Since Zcash uses a flat fee, omit the field (or use a library-appropriate sentinel).
- const { rawUnsignedTx } = await this.prepareTx({ + const { rawUnsignedTx } = await this.prepareTx({ ...params, sender, - feeRate: 0, // Ignored for Zcash })If
prepareTxrequires a value, pass a known-good one (e.g., the lower bound) instead of 0.
62-64: Harden parsing ofrawUnsignedTxand accept object input.
rawUnsignedTxsounds like hex in other UTXO flows. Here it’s JSON; add a defensive parse with a clear error to prevent brittle failures if providers change.- const txData = JSON.parse(rawUnsignedTx) + let txData: any + try { + txData = + typeof rawUnsignedTx === 'string' ? JSON.parse(rawUnsignedTx) : rawUnsignedTx + } catch (e) { + throw new Error('Invalid rawUnsignedTx: expected JSON string/object for Zcash provider') + }
79-100: Clear limitation message is good; consider a custom error type for easier handling upstream.A distinct error class (e.g.,
LedgerSigningUnsupportedError) allows callers to branch logic cleanly.Example (outside this hunk):
export class LedgerSigningUnsupportedError extends Error { constructor(message?: string) { super(message ?? 'Ledger signing unsupported for Zcash without raw hex.') this.name = 'LedgerSigningUnsupportedError' } }Then:
- throw new Error( + throw new LedgerSigningUnsupportedError( 'Zcash Ledger transfers require raw transaction data that is not available from current data providers. ' + ...packages/xchain-zcash/__e2e__/zcash-ledger-client.e2e.ts (2)
49-53: Add a non-brittle assertion to balance test and guard for provider availabilityCurrently only logs balance. Assert expected shape without enforcing a nonzero balance, and optionally skip if providers aren’t configured.
it('get balance', async () => { const address = await zcashClient.getAddressAsync() const balance = await zcashClient.getBalance(address) - console.log('balance', balance[0].amount.amount().toString()) + expect(Array.isArray(balance)).toBe(true) + if (balance.length > 0) { + expect(balance[0]).toHaveProperty('asset') + expect(balance[0]).toHaveProperty('amount') + } })
37-41: Minor: tighten address assertion to reduce false positivesOptional: Assert legacy transparent prefix more precisely (t1 or t3) on mainnet, or allow tm/t2 on testnet depending on selectedNetwork. This prevents accidentally matching unrelated strings.
Example (mainnet):
-expect(address).toMatch(/^t/) +expect(address).toMatch(/^t[13]/)Example (testnet):
-expect(address).toMatch(/^t/) +expect(address).toMatch(/^t[m2]/)If you keep env-selectable network, branch the expectation accordingly.
packages/xchain-zcash/__tests__/clientLedger.test.ts (2)
71-76: Strengthen asset info assertionsAssert expected ZEC decimals (8) to catch regressions in asset metadata.
const assetInfo = client.getAssetInfo() expect(assetInfo).toBeDefined() expect(assetInfo.asset).toBeDefined() - expect(assetInfo.decimal).toBeDefined() + expect(assetInfo.decimal).toBe(8)
78-82: Fix misleading comment and add a negative case for address validationThe address in this test is a mainnet t1 address, not testnet. Also add a negative check to make the test meaningful.
- it('should inherit validateAddress from base client', () => { - // Test with a valid Zcash testnet address format - const isValid = client.validateAddress('t1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9') - expect(typeof isValid).toBe('boolean') - }) + it('should inherit validateAddress from base client', () => { + // Valid Zcash mainnet transparent address + expect(client.validateAddress('t1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9')).toBe(true) + // Clearly invalid + expect(client.validateAddress('not-an-address')).toBe(false) + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/green-peaches-thank.md(1 hunks)packages/xchain-zcash/__e2e__/client.e2e.ts(2 hunks)packages/xchain-zcash/__e2e__/zcash-ledger-client.e2e.ts(1 hunks)packages/xchain-zcash/__tests__/clientLedger.test.ts(1 hunks)packages/xchain-zcash/jest.config.e2e.mjs(1 hunks)packages/xchain-zcash/package.json(1 hunks)packages/xchain-zcash/src/clientLedger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/xchain-zcash/__e2e__/zcash-ledger-client.e2e.ts (2)
packages/xchain-zcash/src/const.ts (3)
zcashExplorerProviders(33-37)NownodesProviders(47-51)AssetZEC(20-20)packages/xchain-util/src/asset.ts (1)
assetToBase(146-152)
packages/xchain-zcash/src/clientLedger.ts (3)
packages/xchain-util/src/asset.ts (1)
Address(18-18)packages/xchain-client/src/types.ts (1)
TxHash(63-63)packages/xchain-client/src/feeRates.ts (1)
checkFeeBounds(35-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
packages/xchain-zcash/__e2e__/client.e2e.ts (2)
25-32: LGTM: trailing comma improves diffs and formatting stability.No functional change, and consistent with common TS style.
35-41: LGTM: same here on trailing comma.Keeps diffs cleaner for future edits.
packages/xchain-zcash/src/clientLedger.ts (2)
24-31: Use the object-style constructor for Zcash (no factory method required)Your current instantiation in
packages/xchain-zcash/src/clientLedger.ts(lines 24–31):this.app = new AppBtc({ transport: this.transport, currency: 'zcash' })is exactly the supported pattern in
@ledgerhq/hw-app-btc@^6.28.6.
- There is no
createAppBtc(…)factory method;- Omitting the
{ currency: 'zcash' }option will default to Bitcoin;- You should only pass
currency: 'zcash'into the main constructor—not into per-call methods likesplitTransactionorcreatePaymentTransactionNew.No changes are needed here. Great job aligning with the official LedgerJS documentation!
39-46: I haven’t yet received the script results—please share the output so I can confirm whethervalidateAddressandgetNetworkexist and adjust the recommendation accordingly.packages/xchain-zcash/__tests__/clientLedger.test.ts (2)
29-31: Good negative test for unsupported sync address methodThis aligns with the Ledger-backed design and protects against accidental sync usage.
85-98: LGTM: fee-rate guarding behavior is correct for flat-fee ZcashExplicitly throwing for fee rates and fees with rates matches the flat-fee model and reduces API ambiguity.
| const defaultZECParams: UtxoClientParams = { | ||
| network: Network.Mainnet, | ||
| phrase: '', | ||
| explorerProviders: zcashExplorerProviders, | ||
| dataProviders: [NownodesProviders], | ||
| rootDerivationPaths: { | ||
| [Network.Mainnet]: `m/44'/133'/0'/0/`, | ||
| [Network.Testnet]: `m/44'/1'/0'/0/`, | ||
| [Network.Stagenet]: `m/44'/133'/0'/0/`, | ||
| }, | ||
| feeBounds: { | ||
| lower: LOWER_FEE_BOUND, | ||
| upper: UPPER_FEE_BOUND, | ||
| }, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid mainnet by default in tests; allow network selection via env
Mainnet usage in tests risks real funds and depends on mainnet-only providers. Default to testnet and allow overrides via an env var (ZCASH_NETWORK). Keep providers as-is; the underlying client should skip undefined per-network entries.
-const defaultZECParams: UtxoClientParams = {
- network: Network.Mainnet,
+const selectedNetwork =
+ (process.env.ZCASH_NETWORK as keyof typeof Network) ? Network[process.env.ZCASH_NETWORK as keyof typeof Network] : Network.Testnet
+
+const defaultZECParams: UtxoClientParams = {
+ network: selectedNetwork,
phrase: '',
explorerProviders: zcashExplorerProviders,
dataProviders: [NownodesProviders],
rootDerivationPaths: {
- [Network.Mainnet]: `m/44'/133'/0'/0/`,
- [Network.Testnet]: `m/44'/1'/0'/0/`,
- [Network.Stagenet]: `m/44'/133'/0'/0/`,
+ [Network.Mainnet]: `m/44'/133'/0'/0/`,
+ [Network.Testnet]: `m/44'/1'/0'/0/`,
+ [Network.Stagenet]: `m/44'/133'/0'/0/`,
},
feeBounds: {
lower: LOWER_FEE_BOUND,
upper: UPPER_FEE_BOUND,
},
}Please confirm your explorer/data providers correctly support the selected network in your environment.
🏁 Script executed:
#!/usr/bin/env bash
# Print the first 200 lines of the E2E test to inspect default network usage
sed -n '1,200p' packages/xchain-zcash/__e2e__/zcash-ledger-client.e2e.tsLength of output: 2277
🏁 Script executed:
#!/usr/bin/env bash
echo "Listing all E2E test files and default network usage"
find packages -type f -path "*/__e2e__/*.e2e.ts" | while read f; do
echo "=== $f ==="
sed -n '1,30p' "$f"
echo
done
echo "Inspecting const.ts in xchain-zcash"
sed -n '1,200p' packages/xchain-zcash/src/const.tsLength of output: 71803
Ensure Testnet Data Provider is Configured Before Defaulting to Testnet in E2E Tests
The current NownodesProviders mapping in packages/xchain-zcash/src/const.ts sets Network.Testnet to undefined, so if you switch the default network to Testnet, balance lookups and transfers will fail due to the missing data provider.
• packages/xchain-zcash/src/const.ts
- Update
NownodesProvidersto include a Testnet provider instance, for example:const testnetNownodesProvider = new NownodesProvider( 'https://testnet-nodes.nownodes.io/api/v2', ZECChain, AssetZEC, ZEC_DECIMAL, process.env.NOWNODES_API_KEY || '' ) export const NownodesProviders: UtxoOnlineDataProviders = { [Network.Testnet]: testnetNownodesProvider, [Network.Stagenet]: mainnetNownodesProvider, [Network.Mainnet]: mainnetNownodesProvider, }
• packages/xchain-zcash/e2e/zcash-ledger-client.e2e.ts
- Introduce dynamic network selection, defaulting to Testnet when
ZCASH_NETWORKis not set:- const defaultZECParams: UtxoClientParams = { - network: Network.Mainnet, + const selectedNetwork = process.env.ZCASH_NETWORK && (Network as any)[process.env.ZCASH_NETWORK] + ? (Network as any)[process.env.ZCASH_NETWORK] + : Network.Testnet + + const defaultZECParams: UtxoClientParams = { + network: selectedNetwork, phrase: '', explorerProviders: zcashExplorerProviders, dataProviders: [NownodesProviders], rootDerivationPaths: { [Network.Mainnet]: `m/44'/133'/0'/0/`, [Network.Testnet]: `m/44'/1'/0'/0/`, [Network.Stagenet]: `m/44'/133'/0'/0/`, }, feeBounds: { lower: LOWER_FEE_BOUND, upper: UPPER_FEE_BOUND, }, }
• Validation Steps
- Confirm that the updated
NownodesProviders[Testnet]works for balance fetch and transaction broadcast. - Run the E2E suite against Testnet (
ZCASH_NETWORK=Testnet) to ensuregetBalanceandtransfersucceed.
🤖 Prompt for AI Agents
In packages/xchain-zcash/__e2e__/zcash-ledger-client.e2e.ts around lines 11-25,
the test currently hardcodes Mainnet and uses NownodesProviders that leave
Network.Testnet undefined; add dynamic network selection (read ZCASH_NETWORK env
var and default to Network.Testnet) so tests run against Testnet by default, and
update packages/xchain-zcash/src/const.ts to create and export a Testnet
NownodesProvider instance (configured with the testnet nownodes URL and API key
from env) and assign it to NownodesProviders[Network.Testnet] so balance lookups
and broadcasts succeed when Testnet is selected.
| it('transfer', async () => { | ||
| try { | ||
| const to = await zcashClient.getAddressAsync(1) | ||
| const amount = assetToBase(assetAmount('0.00002')) | ||
| const txid = await zcashClient.transfer({ | ||
| asset: AssetZEC, | ||
| recipient: to, | ||
| amount, | ||
| memo: 'test', | ||
| }) | ||
| console.log(JSON.stringify(txid, null, 2)) | ||
| } catch (err) { | ||
| console.error('ERR running test', err) | ||
| fail() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert the expected signing limitation error instead of failing the test
Transfers are not production-ready and should surface a clear error about missing raw transaction hex. Assert this error rather than failing generically. Also, specify ZEC decimals to avoid implicit assumptions.
- it('transfer', async () => {
- try {
- const to = await zcashClient.getAddressAsync(1)
- const amount = assetToBase(assetAmount('0.00002'))
- const txid = await zcashClient.transfer({
- asset: AssetZEC,
- recipient: to,
- amount,
- memo: 'test',
- })
- console.log(JSON.stringify(txid, null, 2))
- } catch (err) {
- console.error('ERR running test', err)
- fail()
- }
- })
+ it('transfer should reject with missing raw tx hex limitation', async () => {
+ const to = await zcashClient.getAddressAsync(1)
+ const amount = assetToBase(assetAmount('0.00002', 8)) // ZEC has 8 decimals
+ await expect(
+ zcashClient.transfer({
+ asset: AssetZEC,
+ recipient: to,
+ amount,
+ memo: 'test',
+ }),
+ ).rejects.toThrow(/raw\s+(transaction|tx)\s+(hex|data)/i)
+ })If you prefer keeping the manual try/catch, at least assert the message and remove fail().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('transfer', async () => { | |
| try { | |
| const to = await zcashClient.getAddressAsync(1) | |
| const amount = assetToBase(assetAmount('0.00002')) | |
| const txid = await zcashClient.transfer({ | |
| asset: AssetZEC, | |
| recipient: to, | |
| amount, | |
| memo: 'test', | |
| }) | |
| console.log(JSON.stringify(txid, null, 2)) | |
| } catch (err) { | |
| console.error('ERR running test', err) | |
| fail() | |
| } | |
| }) | |
| it('transfer should reject with missing raw tx hex limitation', async () => { | |
| const to = await zcashClient.getAddressAsync(1) | |
| const amount = assetToBase(assetAmount('0.00002', 8)) // ZEC has 8 decimals | |
| await expect( | |
| zcashClient.transfer({ | |
| asset: AssetZEC, | |
| recipient: to, | |
| amount, | |
| memo: 'test', | |
| }), | |
| ).rejects.toThrow(/raw\s+(transaction|tx)\s+(hex|data)/i) | |
| }) |
🤖 Prompt for AI Agents
In packages/xchain-zcash/__e2e__/zcash-ledger-client.e2e.ts around lines 55 to
70, the test currently catches any error and calls fail(); instead assert the
expected "missing raw transaction hex" (signing limitation) error message so the
test passes when the client correctly indicates transfers are not supported, and
remove the generic fail() and console.error; also make the amount creation
explicit by specifying ZEC decimals (8) when calling assetAmount/assetToBase so
the test does not rely on implicit decimals. Ensure the test asserts the error
message/content (or uses an expect().rejects pattern) rather than treating any
thrown error as a test failure.
| // Mock transport for testing | ||
| const mockTransport = { | ||
| exchange: jest.fn(), | ||
| setExchangeTimeout: jest.fn(), | ||
| close: jest.fn(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize transport mock: include send and/or avoid hitting transport via spies
Tests that fail with “this.transport.send is not a function” are brittle because they depend on internal implementation details. Add a no-op send on the mock to avoid this class of failures.
const mockTransport = {
- exchange: jest.fn(),
+ exchange: jest.fn(),
+ send: jest.fn(), // add to stabilize tests that call into App
setExchangeTimeout: jest.fn(),
close: jest.fn(),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Mock transport for testing | |
| const mockTransport = { | |
| exchange: jest.fn(), | |
| setExchangeTimeout: jest.fn(), | |
| close: jest.fn(), | |
| } | |
| // Mock transport for testing | |
| const mockTransport = { | |
| exchange: jest.fn(), | |
| send: jest.fn(), // add to stabilize tests that call into App | |
| setExchangeTimeout: jest.fn(), | |
| close: jest.fn(), | |
| } |
🤖 Prompt for AI Agents
In packages/xchain-zcash/__tests__/clientLedger.test.ts around lines 3 to 8, the
transport mock lacks a send method which causes brittle failures like
"this.transport.send is not a function"; add a no-op send function to the mock
(e.g., send: jest.fn() or send: () => Promise.resolve()) or adjust tests to
avoid spying on internal transport.send—ensure the mock implements send so tests
don't hit missing-method errors.
| it('should throw error when transport is not properly configured', async () => { | ||
| const mockAmount = { | ||
| amount: () => ({ toNumber: () => 1000000 }), | ||
| } | ||
|
|
||
| const transferParams = { | ||
| recipient: 't1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9', | ||
| amount: mockAmount, | ||
| memo: 'test', | ||
| } | ||
|
|
||
| // Since we're using a mock transport, it should fail at the transport level | ||
| await expect(client.transfer(transferParams as Parameters<typeof client.transfer>[0])).rejects.toThrow( | ||
| 'this.transport.send is not a function', | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert the documented signing limitation instead of relying on a TypeError
Per PR objectives, transfer should fail with a clear “raw tx hex” limitation. Stub getAddressAsync to avoid transport usage and assert on that error, not on a TypeError about send.
- it('should throw error when transport is not properly configured', async () => {
- const mockAmount = {
- amount: () => ({ toNumber: () => 1000000 }),
- }
-
- const transferParams = {
- recipient: 't1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9',
- amount: mockAmount,
- memo: 'test',
- }
-
- // Since we're using a mock transport, it should fail at the transport level
- await expect(client.transfer(transferParams as Parameters<typeof client.transfer>[0])).rejects.toThrow(
- 'this.transport.send is not a function',
- )
- })
+ it('should surface Ledger signing limitation when raw tx hex is unavailable', async () => {
+ // Avoid transport by stubbing sender derivation
+ jest.spyOn(client, 'getAddressAsync').mockResolvedValue('t1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9')
+ const mockAmount = { amount: () => ({ toNumber: () => 1000000 }) }
+ const transferParams = {
+ recipient: 't1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9',
+ amount: mockAmount,
+ memo: 'test',
+ // include asset if transfer expects it
+ asset: { chain: 'ZEC', symbol: 'ZEC', ticker: 'ZEC', type: 'NATIVE' } as any,
+ }
+ await expect(client.transfer(transferParams as Parameters<typeof client.transfer>[0])).rejects.toThrow(
+ /raw\s+(transaction|tx)\s+(hex|data)/i,
+ )
+ })If you prefer, I can update the test to import AssetZEC instead of using a loose cast.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should throw error when transport is not properly configured', async () => { | |
| const mockAmount = { | |
| amount: () => ({ toNumber: () => 1000000 }), | |
| } | |
| const transferParams = { | |
| recipient: 't1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9', | |
| amount: mockAmount, | |
| memo: 'test', | |
| } | |
| // Since we're using a mock transport, it should fail at the transport level | |
| await expect(client.transfer(transferParams as Parameters<typeof client.transfer>[0])).rejects.toThrow( | |
| 'this.transport.send is not a function', | |
| ) | |
| }) | |
| it('should surface Ledger signing limitation when raw tx hex is unavailable', async () => { | |
| // Avoid transport by stubbing sender derivation | |
| jest.spyOn(client, 'getAddressAsync').mockResolvedValue('t1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9') | |
| const mockAmount = { amount: () => ({ toNumber: () => 1000000 }) } | |
| const transferParams = { | |
| recipient: 't1d4ZFodUN3sJz1zL6SfKSV6kmkwYm8N5s9', | |
| amount: mockAmount, | |
| memo: 'test', | |
| // include asset if transfer expects it | |
| asset: { chain: 'ZEC', symbol: 'ZEC', ticker: 'ZEC', type: 'NATIVE' } as any, | |
| } | |
| await expect(client.transfer(transferParams as Parameters<typeof client.transfer>[0])).rejects.toThrow( | |
| /raw\s+(transaction|tx)\s+(hex|data)/i, | |
| ) | |
| }) |
🤖 Prompt for AI Agents
In packages/xchain-zcash/__tests__/clientLedger.test.ts around lines 52 to 67,
the test currently relies on a TypeError from a mocked transport; instead stub
the client's getAddressAsync (or mock the ledger method that returns the
address) so transport.send is not invoked and the flow reaches the documented
signing limitation; then assert the rejection message matches the expected "raw
tx hex" limitation (e.g., use expect(...).rejects.toThrow('raw tx hex')) so the
test verifies the intended error; optionally replace the loose cast with an
imported AssetZEC when constructing transferParams for clarity.
| "directory": "release/package" | ||
| }, | ||
| "devDependencies": { | ||
| "@ledgerhq/hw-transport-node-hid": "^6.28.6", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dev dep looks fine, but runtime dependency on '@ledgerhq/hw-app-btc' is missing.
This package imports '@ledgerhq/hw-app-btc' in src/clientLedger.ts, but it’s not declared in dependencies. Relying on workspace hoisting is brittle and will break consumers.
Apply this diff to add the runtime dependency (version aligned to your transport to avoid API drift):
"dependencies": {
"@bitcoin-js/tiny-secp256k1-asmjs": "^2.2.3",
"@mayaprotocol/zcash-js": "1.0.7",
"@scure/bip32": "^1.7.0",
"@xchainjs/xchain-client": "workspace:*",
"@xchainjs/xchain-crypto": "workspace:*",
"@xchainjs/xchain-util": "workspace:*",
"@xchainjs/xchain-utxo": "workspace:*",
"@xchainjs/xchain-utxo-providers": "workspace:*",
- "ecpair": "2.1.0"
+ "ecpair": "2.1.0",
+ "@ledgerhq/hw-app-btc": "^6.28.6"
},Optionally, if you only need types from '@ledgerhq/hw-transport', add it as a devDependency:
"devDependencies": {
"@ledgerhq/hw-transport-node-hid": "^6.28.6",
+ "@ledgerhq/hw-transport": "^6.28.6",
"@types/blake2b-wasm": "^2.4.3",Verify there’s no accidental transitive reliance:
🏁 Script executed:
#!/bin/bash
# Check direct imports vs declared deps for '@ledgerhq/hw-app-btc'
rg -n "@ledgerhq/hw-app-btc" packages/xchain-zcash/src | cat
jq -r '.dependencies | keys[]' packages/xchain-zcash/package.json | rg -n "^@ledgerhq/hw-app-btc$" -n || echo "MISSING in dependencies"Length of output: 307
Add missing runtime dependency for '@ledgerhq/hw-app-btc'
The packages/xchain-zcash package imports @ledgerhq/hw-app-btc but does not declare it under "dependencies", causing builds or consumers to break when workspace hoisting isn’t in effect (verified via ripgrep and jq).
• packages/xchain-zcash/src/clientLedger.ts (line 1):
import AppBtc from '@ledgerhq/hw-app-btc'
• packages/xchain-zcash/package.json: no "@ledgerhq/hw-app-btc" key under "dependencies"
Apply this diff to ensure the runtime dependency is declared and version-aligned with your transport:
"dependencies": {
"@bitcoin-js/tiny-secp256k1-asmjs": "^2.2.3",
"@mayaprotocol/zcash-js": "1.0.7",
"@scure/bip32": "^1.7.0",
"@xchainjs/xchain-client": "workspace:*",
"@xchainjs/xchain-crypto": "workspace:*",
"@xchainjs/xchain-util": "workspace:*",
"@xchainjs/xchain-utxo": "workspace:*",
"@xchainjs/xchain-utxo-providers": "workspace:*",
"ecpair": "2.1.0",
+ "@ledgerhq/hw-app-btc": "^6.28.6"
},Optionally, if you depend only on types from @ledgerhq/hw-transport, declare it as a dev dependency:
"devDependencies": {
"@ledgerhq/hw-transport-node-hid": "^6.28.6",
+ "@ledgerhq/hw-transport": "^6.28.6",
"@types/blake2b-wasm": "^2.4.3",
…
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@ledgerhq/hw-transport-node-hid": "^6.28.6", | |
| ++ b/packages/xchain-zcash/package.json | |
| @@ "dependencies": { | |
| "@bitcoin-js/tiny-secp256k1-asmjs": "^2.2.3", | |
| "@mayaprotocol/zcash-js": "1.0.7", | |
| "@scure/bip32": "^1.7.0", | |
| "@xchainjs/xchain-client": "workspace:*", | |
| "@xchainjs/xchain-crypto": "workspace:*", | |
| "@xchainjs/xchain-util": "workspace:*", | |
| "@xchainjs/xchain-utxo": "workspace:*", | |
| "@xchainjs/xchain-utxo-providers": "workspace:*", | |
| "ecpair": "2.1.0", | |
| "@ledgerhq/hw-app-btc": "^6.28.6" | |
| }, | |
| @@ "devDependencies": { | |
| "@ledgerhq/hw-transport-node-hid": "^6.28.6", | |
| "@ledgerhq/hw-transport": "^6.28.6", | |
| "@types/blake2b-wasm": "^2.4.3", | |
| … | |
| } |
🤖 Prompt for AI Agents
In packages/xchain-zcash/package.json at line ~51, the package imports
'@ledgerhq/hw-app-btc' (see packages/xchain-zcash/src/clientLedger.ts) but
doesn't declare it as a runtime dependency; add "@ledgerhq/hw-app-btc":
"^6.28.6" (or match the same version range used for
"@ledgerhq/hw-transport-node-hid") to the "dependencies" section so
consumers/builds without workspace hoisting can resolve it; if you only need
types from a transport package instead, add that package as a devDependency
instead.
| // Get the Ledger BTC application instance configured for Zcash | ||
| public async getApp(): Promise<AppBtc> { | ||
| if (this.app) { | ||
| return this.app | ||
| } | ||
| this.app = new AppBtc({ transport: this.transport, currency: 'zcash' }) | ||
| return this.app | ||
| } |
There was a problem hiding this comment.
Incorrect AppBtc construction — will produce this.transport.send is not a function.
new AppBtc({ transport, currency: 'zcash' }) passes an object where a Transport instance is expected. This will break calls like getWalletPublicKey. Use the Transport instance directly; if you need Zcash coin params, use the version of the API that supports coin options (factory), otherwise stick to plain constructor and pass coin-specific options at signing time.
Apply this minimal fix:
- this.app = new AppBtc({ transport: this.transport, currency: 'zcash' })
+ this.app = new AppBtc(this.transport)Additionally, strongly consider typing the transport to catch this at compile time:
// at top of file
import type Transport from '@ledgerhq/hw-transport'
// fields
private transport: TransportIf your installed '@ledgerhq/hw-app-btc' supports a factory with currency (varies by version), prefer that instead:
// Pseudocode (verify availability in your version)
import { createAppBtc } from '@ledgerhq/hw-app-btc'
this.app = createAppBtc(this.transport, { currency: 'zcash' })See the verification block below to confirm which API your version exposes.
🤖 Prompt for AI Agents
In packages/xchain-zcash/src/clientLedger.ts around lines 24 to 31, the AppBtc
is being constructed with an object containing transport which causes runtime
errors like "this.transport.send is not a function"; replace the current
construction so you pass the Transport instance directly to AppBtc (or use the
library's factory that accepts coin options if available), i.e. call the
constructor/factory with the transport variable rather than an object, and add a
Transport type import and type the transport field (import type Transport from
'@ledgerhq/hw-transport' and declare private transport: Transport) so TypeScript
catches incorrect usage.
| checkFeeBounds(this.feeBounds, tx.fee) | ||
|
|
||
| // LIMITATION: Zcash Ledger transaction signing requires raw transaction hex data |
There was a problem hiding this comment.
Fee bounds check is using total fee instead of a fee rate — likely incorrect.
checkFeeBounds expects a fee rate, but tx.fee is an absolute fee (zatoshis). This can throw incorrectly and is redundant if prepareTx already validated fees.
Apply this diff to remove the erroneous check:
- // Check fee bounds (already done in prepareTx but double-check)
- checkFeeBounds(this.feeBounds, tx.fee)If you do want a bounds check here, compute a synthetic fee rate from tx size and fee (if available) and compare like-for-like.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check fee bounds (already done in prepareTx but double-check) | |
| checkFeeBounds(this.feeBounds, tx.fee) |
🤖 Prompt for AI Agents
In packages/xchain-zcash/src/clientLedger.ts around lines 76 to 78, the code
calls checkFeeBounds(this.feeBounds, tx.fee) but checkFeeBounds expects a fee
rate while tx.fee is an absolute amount; remove this erroneous call (since
prepareTx already validated fees) or if you want to keep a guard, compute a
synthetic fee rate by dividing tx.fee by tx.size (or available tx byte size) and
pass that rate to checkFeeBounds instead, ensuring like-for-like comparison.
1 participant
Limited Production Readiness
Transfers: Not production ready for transfers
Summary by CodeRabbit