Feat/zcash ledger #1561
Feat/zcash ledger #1561
Conversation
📝 WalkthroughWalkthroughIntroduces a Ledger-backed Zcash client with PSBT building/signing, adds Zcash e2e tests (including Ledger), updates Zcash dependencies and module declarations, adjusts root resolutions, and modifies the NowNodes UTXO provider to fetch raw transactions per UTXO (adding txHex). Includes a changeset entry and small e2e test index change. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
packages/xchain-zcash/__e2e__/client.e2e.ts (1)
14-44: Consider adding assertions for test reliability.While these e2e tests currently execute operations successfully, they lack assertions (
expect()statements) to validate correctness. This means tests will pass even if operations return incorrect or unexpected values.Consider adding assertions to each test. For example:
it('Should get address', async () => { const address = await client.getAddressAsync(0) console.log('address', address) + expect(address).toBeTruthy() + expect(typeof address).toBe('string') })it('Should get balance', async () => { const address = await client.getAddressAsync(0) const balance = await client.getBalance(address) console.log('Balance', balance[0].amount.amount().toString()) console.log(balance[0].asset) + expect(balance).toBeDefined() + expect(Array.isArray(balance)).toBe(true) })it('Should transfer TX without memo', async () => { const address = await client.getAddressAsync(1) const hash = await client.transfer({ walletIndex: 0, amount: assetToBase(assetAmount('0.1', 8)), recipient: address, }) console.log('hash', hash) + expect(hash).toBeTruthy() + expect(typeof hash).toBe('string') })packages/xchain-zcash/package.json (1)
47-47: Verify coinselect version.The specified version
3.1.12is from April 2020. Based on learnings, the latest published version is3.1.13(June 2022). Consider updating to the latest patch version for potential bug fixes.Apply this diff to update to the latest version:
- "coinselect": "3.1.12", + "coinselect": "3.1.13",Based on learnings.
packages/xchain-zcash/__e2e__/clientLedger.e2e.ts (2)
9-16: Add cleanup inafterAllhook.The Ledger transport is created in
beforeAllbut never explicitly closed. This could leave the device connection open between test runs.Add an
afterAllhook to properly close the transport:afterAll(async () => { await client.transport.close() })
20-20: Consider removing console.log statements.The test contains several
console.logstatements that should be removed or replaced with a proper test logger for cleaner test output in CI/CD environments.Also applies to: 32-33, 47-47, 60-60
packages/xchain-zcash/src/ledger-btc.js (1)
1-24: Document rationale for including legacy Ledger code.This file contains legacy Ledger toolkit code from 2016-2019. Consider:
- Documenting why this legacy code is needed instead of using modern
@ledgerhq/hw-app-btcAPIs directly (which is already a dependency).- Adding a README or comment explaining the relationship between this file and the hw-app-btc package.
- Evaluating whether the modern hw-app-btc API can replace this legacy implementation to reduce maintenance burden.
Would you like me to investigate whether modern
@ledgerhq/hw-app-btcAPIs (version 10.11.2 in your dependencies) can replace this legacy implementation?packages/xchain-zcash/src/clientLedger.ts (1)
124-126: Remove redundant skip logicThe
if (hasOutputScript && !compiledMemo) { continue }branch is unreachable—script outputs are only ever added whencompiledMemois truthy. Remove these lines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/xchain-utxo-providers/src/providers/nownodes/nownodes-data-provider.ts(1 hunks)packages/xchain-zcash/__e2e__/client.e2e.ts(1 hunks)packages/xchain-zcash/__e2e__/clientLedger.e2e.ts(1 hunks)packages/xchain-zcash/package.json(1 hunks)packages/xchain-zcash/src/clientLedger.ts(1 hunks)packages/xchain-zcash/src/ledger-btc.js(1 hunks)packages/xchain-zcash/src/modules.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
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)
packages/xchain-zcash/__e2e__/clientLedger.e2e.ts (1)
packages/xchain-util/src/asset.ts (1)
assetToBase(146-152)
🪛 Biome (2.1.2)
packages/xchain-zcash/src/ledger-btc.js
[error] 704-704: Shouldn't redeclare 'buffer'. Consider to delete it or rename it.
'buffer' is defined here:
(lint/suspicious/noRedeclare)
[error] 710-710: Shouldn't redeclare 'buffer'. Consider to delete it or rename it.
'buffer' is defined here:
(lint/suspicious/noRedeclare)
[error] 724-724: Shouldn't redeclare 'transaction'. Consider to delete it or rename it.
'transaction' is defined here:
(lint/suspicious/noRedeclare)
[error] 724-724: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 745-745: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 813-813: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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 (17)
packages/xchain-zcash/__e2e__/client.e2e.ts (1)
15-16: LGTM! More intuitive test setup.Changing the index from 1 to 0 makes this test more intuitive as a basic address retrieval test, and aligns it with the "get balance" test on line 20 which also uses index 0.
packages/xchain-zcash/src/modules.d.ts (1)
1-2: LGTM!The module augmentation for
coinselect/accumulative.jsis correctly declared, enabling TypeScript support for the untyped JavaScript module used in the Ledger client implementation.packages/xchain-zcash/__e2e__/clientLedger.e2e.ts (2)
40-50: LGTM!The transfer test correctly validates transaction creation and broadcasting without a memo. The use of real amounts (0.1 ZEC) is appropriate for end-to-end testing.
52-63: Skipped test aligns with PR objectives.The memo test is appropriately skipped, consistent with the PR description indicating that memo functionality is part of the "Remaining / TODO" work. Consider adding a TODO comment explaining the skip reason.
You can optionally add a clarifying comment:
// TODO: Enable once memo support is fully implemented (see PR objectives) it.skip('Should transfer TX with memo', async () => {packages/xchain-zcash/src/clientLedger.ts (13)
1-10: LGTM!The imports are well-organized and include all necessary types and functions for the Ledger-based Zcash client implementation.
14-14: LGTM!The
UtxoLedgerClientParamstype correctly extendsUtxoClientParamswith the required Ledger transport, maintaining a clean API surface.
16-23: LGTM!The constructor properly stores the Ledger transport, and the lazy initialization pattern for
ledgerAppis appropriate for managing the Ledger connection lifecycle.
25-33: LGTM!The lazy initialization pattern for the Ledger app is correctly implemented, with proper currency configuration for Zcash.
35-45: LGTM!The address retrieval methods correctly handle the Ledger's asynchronous nature. The synchronous method appropriately throws an error, and the async method properly derives the address using the legacy format suitable for Zcash.
67-86: LGTM!The UTXO selection logic using coinselect is correctly implemented, with proper handling of the recipient output and optional memo script.
88-103: LGTM!The PSBT initialization correctly sets up Zcash-specific parameters including the NU5 consensus branch ID and version 455, which are appropriate for Zcash transaction construction.
135-140: LGTM!The
buildTxmethod correctly returns all necessary components (PSBT, UTXOs, and inputs) for the subsequent signing process.
148-159: LGTM!The transfer initialization correctly retrieves the Ledger app, validates fee bounds, and derives the sender address from the wallet index.
176-197: LGTM!The Ledger signing flow correctly handles Zcash-specific parameters including non-segwit mode, expiry height, and the 'zcash' additional parameter.
63-63: VerifyspendPendingUTXOhandlingThe Keystore and Ledger clients both hard-code
scanUTXOs(..., true)instead of exposing thespendPendingUTXOoption that the coreClientdoes. Decide whether Ledger/Keystore should surface this flag; if not, update comments to remove the misleading reference.
164-174: Verify Zcash hash encoding
Confirm that@ledgerhq/hw-app-btc@^10.11.2actually uses BLAKE2 (not SHA-256) when signing Zcash transactions; if it doesn’t, track and implement the missing BLAKE2 hashing before considering this complete.
108-118: EnsuretxHexis provided for non-witness UTXOs or handle missing cases
The UTXO type makestxHexoptional; if you calladdInputwithnonWitnessUtxo: undefined, PSBT construction will fail silently. Confirm that your provider always setstxHexwhenwitnessUtxois absent, or add a guard to throw an error/fallback whenutxo.txHexis undefined.
packages/xchain-utxo-providers/src/providers/nownodes/nownodes-data-provider.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/xchain-zcash/src/clientLedger.ts (2)
121-123: Clarify the output script filtering logic.This logic skips outputs with a script but no compiled memo. Since coinselect shouldn't generate script outputs (only address-based outputs or change), this condition seems unusual. If the intent is to filter out OP_RETURN outputs that aren't memos, the logic should be more explicit.
Consider adding a comment explaining when this condition would be true, or simplify the logic if this case cannot occur:
- if (hasOutputScript && !compiledMemo) { + // Skip non-memo script outputs (if any) + if (hasOutputScript && !compiledMemo) { continue }
172-173: Clarify expiryHeight = 0 usage (packages/xchain-zcash/src/clientLedger.ts:172). Zcash allows 0 (no-expiry per ZIP-203) but UX best practice is default TTL (current height+40). Confirm this intent or switch to the default.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xchain-zcash/src/clientLedger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-zcash/src/clientLedger.ts (5)
packages/xchain-zcash/src/index.ts (1)
ClientLedger(5-5)packages/xchain-zcash/src/client.ts (1)
Client(202-202)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 (1)
packages/xchain-zcash/src/clientLedger.ts (1)
10-10: Ignore suggestion to use Mayaprotocol for UTXO selection.@mayaprotocol/zcash-jsonly exports high-level builders (buildTx, signAndFinalize, etc.) and doesn’t expose a standalone coin-selection API, so the direct use ofcoinselect/accumulative.jsremains necessary here.Likely an incorrect or invalid review comment.
* feat: add tron ledger client * chore: add changeset
* updated decimal cache and added Mayanode-first pool caching with Midgard fallback * add changeset * fix broken test * update from CR comments
* update chainflip sdk to the latest and add broker config * update * added changeset
* feat: add maya ledger client * feat: add ledger-thorchain npm * chore: fix swap e2e test address
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Add changeset pls. |
|
3 participants
DONE
TODO
sha256encoding for tx hash but zcash is usingblake2unlike other utxo chains like BTC, DOGE, LTCmissing inputserror.Summary by CodeRabbit
New Features
Tests
Chores