-
Couldn't load subscription status.
- Fork 108
Zcash ledger implementation #1523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@xchainjs/xchain-zcash': patch | ||
| --- | ||
|
|
||
| adding client ledger |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,71 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import TransportNodeHid from '@ledgerhq/hw-transport-node-hid' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Network } from '@xchainjs/xchain-client' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { assetAmount, assetToBase } from '@xchainjs/xchain-util' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { UtxoClientParams } from '@xchainjs/xchain-utxo' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ClientLedger } from '../src/clientLedger' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { AssetZEC, LOWER_FEE_BOUND, NownodesProviders, UPPER_FEE_BOUND, zcashExplorerProviders } from '../src/const' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| jest.setTimeout(200000) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Zcash Client Ledger', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let zcashClient: ClientLedger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| beforeAll(async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const transport = await TransportNodeHid.create() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| zcashClient = new ClientLedger({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| transport, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...defaultZECParams, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Thorian1te marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('get address async without verification', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const address = await zcashClient.getAddressAsync() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log('address', address) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(address).toMatch(/^t/) // Transparent addresses start with t | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('get address async with verification', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const address = await zcashClient.getAddressAsync(0, true) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log('address', address) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(address).toMatch(/^t/) // Transparent addresses start with t | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Thorian1te marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('get balance', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const address = await zcashClient.getAddressAsync() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const balance = await zcashClient.getBalance(address) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log('balance', balance[0].amount.amount().toString()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+55
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,99 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ClientLedger, defaultZECParams } from '../src' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Mock transport for testing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const mockTransport = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exchange: jest.fn(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setExchangeTimeout: jest.fn(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| close: jest.fn(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Zcash Ledger Client', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let client: ClientLedger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| beforeEach(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client = new ClientLedger({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...defaultZECParams, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| transport: mockTransport, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| afterEach(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| jest.clearAllMocks() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Constructor', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should create a ClientLedger instance', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(client).toBeInstanceOf(ClientLedger) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should throw error for sync getAddress method', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(() => client.getAddress()).toThrow('Sync method not supported for Ledger') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Address Operations', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should have getApp method', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(client.getApp).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(typeof client.getApp).toBe('function') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should have getAddressAsync method', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(client.getAddressAsync).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(typeof client.getAddressAsync).toBe('function') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Transaction Operations', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should have transfer method', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(client.transfer).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(typeof client.transfer).toBe('function') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+52
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Network and Asset Info', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should inherit getAssetInfo from base client', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const assetInfo = client.getAssetInfo() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(assetInfo).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(assetInfo.asset).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(assetInfo.decimal).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('Fee Operations', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should inherit fee methods from base client', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(client.getFees).toBeDefined() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(typeof client.getFees).toBe('function') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should throw error for fee rates (Zcash uses flat fees)', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await expect(client.getFeeRates()).rejects.toThrow('Error Zcash has flat fee. Fee rates not supported') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it('should throw error for fees with rates', async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await expect(client.getFeesWithRates()).rejects.toThrow('Error Zcash has flat fee. Fee rates not supported') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |||||||||||||||||||||||||||||||||||||||||||
| "directory": "release/package" | ||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||
| "devDependencies": { | ||||||||||||||||||||||||||||||||||||||||||||
| "@ledgerhq/hw-transport-node-hid": "^6.28.6", | ||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainDev 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/src/clientLedger.ts (line 1): • packages/xchain-zcash/package.json: no 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 "devDependencies": {
"@ledgerhq/hw-transport-node-hid": "^6.28.6",
+ "@ledgerhq/hw-transport": "^6.28.6",
"@types/blake2b-wasm": "^2.4.3",
…
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| "@types/blake2b-wasm": "^2.4.3", | ||||||||||||||||||||||||||||||||||||||||||||
| "@types/js-yaml": "^4.0.9", | ||||||||||||||||||||||||||||||||||||||||||||
| "@types/lodash": "^4.17.13", | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,29 +1,103 @@ | ||||||
| import { TxHash } from '@xchainjs/xchain-client' | ||||||
| import AppBtc from '@ledgerhq/hw-app-btc' | ||||||
| import { buildTx } from '@mayaprotocol/zcash-js' | ||||||
| import { TxHash, checkFeeBounds } from '@xchainjs/xchain-client' | ||||||
| import { Address } from '@xchainjs/xchain-util' | ||||||
| import { UtxoClientParams } from '@xchainjs/xchain-utxo' | ||||||
|
|
||||||
| import { TxParams, UtxoClientParams } from '@xchainjs/xchain-utxo' | ||||||
| import { Client } from './client' | ||||||
|
|
||||||
| /** | ||||||
| * Custom Ledger Zcash client | ||||||
| */ | ||||||
| class ClientLedger extends Client { | ||||||
| constructor(params: UtxoClientParams) { | ||||||
| // Reference to the Ledger transport object | ||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||
| private transport: any | ||||||
| private app: AppBtc | undefined | ||||||
|
|
||||||
| // Constructor | ||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||
| constructor(params: UtxoClientParams & { transport: any }) { | ||||||
| super(params) | ||||||
| throw Error('Ledger client not supported for Zcash.') | ||||||
| this.transport = params.transport | ||||||
| } | ||||||
|
|
||||||
| public async getApp() { | ||||||
| throw Error('Not implemented.') | ||||||
| // 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 | ||||||
| } | ||||||
|
Comment on lines
+24
to
31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect AppBtc construction — will produce
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 |
||||||
|
|
||||||
| // Get the current address synchronously | ||||||
| getAddress(): string { | ||||||
| throw Error('Not implemented.') | ||||||
| throw Error('Sync method not supported for Ledger') | ||||||
| } | ||||||
|
|
||||||
| async getAddressAsync(): Promise<Address> { | ||||||
| throw Error('Not implemented.') | ||||||
| // Get the current address asynchronously | ||||||
| async getAddressAsync(index = 0, verify = false): Promise<Address> { | ||||||
| const app = await this.getApp() | ||||||
| const result = await app.getWalletPublicKey(this.getFullDerivationPath(index), { | ||||||
| format: 'legacy', | ||||||
| verify, | ||||||
| }) | ||||||
| return result.bitcoinAddress | ||||||
| } | ||||||
|
|
||||||
| async transfer(): Promise<TxHash> { | ||||||
| throw Error('Not implemented.') | ||||||
| // Transfer ZEC from Ledger | ||||||
| async transfer(params: TxParams): Promise<TxHash> { | ||||||
| const fromAddressIndex = params?.walletIndex || 0 | ||||||
|
|
||||||
| // Get sender address | ||||||
| const sender = await this.getAddressAsync(fromAddressIndex) | ||||||
|
|
||||||
| // Prepare transaction using base client method (handles flat fee) | ||||||
| const { rawUnsignedTx } = await this.prepareTx({ | ||||||
| ...params, | ||||||
| sender, | ||||||
| feeRate: 0, // Ignored for Zcash | ||||||
| }) | ||||||
|
|
||||||
| // Parse the transaction data | ||||||
| const txData = JSON.parse(rawUnsignedTx) | ||||||
|
|
||||||
| // Build the actual transaction for signing | ||||||
| const tx = await buildTx( | ||||||
| txData.height, | ||||||
| txData.from, | ||||||
| txData.to, | ||||||
| txData.amount, | ||||||
| txData.utxos, | ||||||
| txData.isMainnet, | ||||||
| txData.memo, | ||||||
| ) | ||||||
|
|
||||||
| // Check fee bounds (already done in prepareTx but double-check) | ||||||
| checkFeeBounds(this.feeBounds, tx.fee) | ||||||
|
|
||||||
|
Comment on lines
+76
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fee bounds check is using total fee instead of a fee rate — likely incorrect.
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
Suggested change
🤖 Prompt for AI Agents |
||||||
| // LIMITATION: Zcash Ledger transaction signing requires raw transaction hex data | ||||||
| // for previous transactions (UTXOs), but Zcash data providers only return | ||||||
| // parsed transaction objects without the raw hex. | ||||||
| // | ||||||
| // This is different from Bitcoin where the prepareTx method fetches and includes | ||||||
| // the raw transaction hex (txHex field) for each UTXO. | ||||||
| // | ||||||
| // To fully implement Zcash Ledger signing, we would need: | ||||||
| // 1. A Zcash data provider that returns raw transaction hex | ||||||
| // 2. Or use the dedicated Zcash Ledger app instead of Bitcoin app | ||||||
| // 3. Or implement a custom serialization from the transaction object | ||||||
|
|
||||||
| throw new Error( | ||||||
| 'Zcash Ledger transfers require raw transaction data that is not available from current data providers. ' + | ||||||
| 'The transaction has been built successfully with fee: ' + | ||||||
| tx.fee + | ||||||
| ' zatoshis. ' + | ||||||
| 'To complete Ledger signing, either:\n' + | ||||||
| '1. Use the keystore client for transfers, or\n' + | ||||||
| '2. Use the dedicated Zcash Ledger app, or\n' + | ||||||
| '3. Implement a data provider that returns raw transaction hex', | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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.
Please confirm your explorer/data providers correctly support the selected network in your environment.
🏁 Script executed:
Length of output: 2277
🏁 Script executed:
Length of output: 71803
Ensure Testnet Data Provider is Configured Before Defaulting to Testnet in E2E Tests
The current
NownodesProvidersmapping inpackages/xchain-zcash/src/const.tssetsNetwork.Testnettoundefined, 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
NownodesProvidersto include a Testnet provider instance, for example:• packages/xchain-zcash/e2e/zcash-ledger-client.e2e.ts
ZCASH_NETWORKis not set:• Validation Steps
NownodesProviders[Testnet]works for balance fetch and transaction broadcast.ZCASH_NETWORK=Testnet) to ensuregetBalanceandtransfersucceed.🤖 Prompt for AI Agents