feat: add more trezor paths, export path type, add ledger bluetooth #710
Conversation
WalkthroughAdds documentation and package metadata updates; expands BIP44 derivation paths; introduces a LedgerInit transport initializer (BLE-first, WebUSB fallback) used by Ledger providers; adapts Trezor providers to use network-based supported paths and extension detection; and exports additional TypeScript types. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant HWwalletManager
participant LedgerProvider
participant LedgerInit
participant BLETransport
participant WebUSBTransport
App->>HWwalletManager: getAddress(network, provider)
HWwalletManager->>LedgerProvider: getAddress(...)
LedgerProvider->>LedgerInit: ensure transport (super.init)
LedgerInit->>BLETransport: isSupported? / create()
alt BLE supported & created
BLETransport-->>LedgerInit: transport instance
LedgerInit->>LedgerProvider: transport ready
BLETransport-->>LedgerInit: disconnect -> clear transport
else BLE unsupported/failed
LedgerInit->>WebUSBTransport: isSupported? / openConnected() or create()
alt WebUSB available
WebUSBTransport-->>LedgerInit: transport instance
LedgerInit->>LedgerProvider: transport ready
else
LedgerInit-->>LedgerProvider: Error (no transport)
end
end
LedgerProvider-->>HWwalletManager: Address / Error
HWwalletManager-->>App: Address / Error
sequenceDiagram
participant App
participant TrezorProvider
participant TrezorConnect
App->>TrezorProvider: getAddress(options)
TrezorProvider->>TrezorProvider: detect isExtension (chrome.runtime)
alt isExtension == true and network not in networkBasedSupportedPaths
TrezorProvider-->>App: Error (unsupported network)
else
TrezorProvider->>TrezorConnect: request address (allow ALL_SUPPORTED_PATHS when not extension)
TrezorConnect-->>TrezorProvider: Address
TrezorProvider-->>App: Address
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
💼 Build Files |
gamalielhere
force-pushed
the
devop/hw-wallets-update
branch
from
7f06ee1 to
dd5709c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/hw-wallets/src/trezor/ethereum/configs.ts (1)
31-65: Consider using spread operator for better readabilityThe comprehensive path list is valuable for broader hardware wallet support. However, the
Object.assignpattern on line 36 could be simplified.- Object.assign({}, bip44Paths.ethereumLedger, { label: "Ethereum - Ledger" }), // specify ledger + { ...bip44Paths.ethereumLedger, label: "Ethereum - Ledger" }, // specify ledgerThis uses the modern spread operator which is more readable and concise.
packages/hw-wallets/src/ledger/ethereum/index.ts (2)
3-3: Fix import spacing for consistencyMinor formatting issue with the import statement.
-import bleTransport from '@ledgerhq/hw-transport-web-ble'; +import bleTransport from "@ledgerhq/hw-transport-web-ble";
42-44: Improve disconnect handler with loggingThe disconnect handler should include logging for better debugging.
transport.on("disconnect", () => { // connection wasnt succesful + console.log("Ledger BLE transport disconnected"); this.transport = null; -}) +});packages/hw-wallets/README.md (5)
23-27: Add language specification to code block and fix import syntax.The code block should specify JavaScript/TypeScript as the language, and the import statement is missing quotes around the module name.
-``` -import HWwalletManager from @enkryptcom/hw-wallets +```javascript +import HWwalletManager from '@enkryptcom/hw-wallets'🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
23-23: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
29-29: Add missing article for clarity.-2. Call methods within class, passing network names and providers. Class automatically handles which wallet to use. +2. Call methods within class, passing network names and providers. The class automatically handles which wallet to use.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: A determiner appears to be missing. Consider inserting it.
Context: ...s, passing network names and providers. Class automatically handles which wallet to u...(AI_EN_LECTOR_MISSING_DETERMINER)
35-35: Simplify wordy phrase.-Returns wallet address based off of the path provided in the `getAddressRequest`. +Returns wallet address based on the path provided in the `getAddressRequest`.🧰 Tools
🪛 LanguageTool
[style] ~35-~35: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...esponse>Returns wallet address based off of the path provided in thegetAddressReq...(EN_WORDINESS_PREMIUM_OFF_OF)
63-68: Convert bare URLs to proper markdown links.For better readability and markdown compliance, convert the bare URLs to proper markdown links.
-`NetworkNames`: https://github.com/enkryptcom/enKrypt/blob/main/packages/types/src/networks.ts#L1 -`getAddressRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L74 -`AddressResponse`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L31 -`SignMessageRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L54 -`SignTransactionRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L65 -`isConnectedRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L78 +[`NetworkNames`](https://github.com/enkryptcom/enKrypt/blob/main/packages/types/src/networks.ts#L1) +[`getAddressRequest`](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L74) +[`AddressResponse`](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L31) +[`SignMessageRequest`](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L54) +[`SignTransactionRequest`](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L65) +[`isConnectedRequest`](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L78)🧰 Tools
🪛 LanguageTool
[uncategorized] ~63-~63: Loose punctuation mark.
Context: ...t connections ### TypesNetworkNames: https://github.com/enkryptcom/enKrypt/b...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
63-63: Bare URL used
null(MD034, no-bare-urls)
64-64: Bare URL used
null(MD034, no-bare-urls)
65-65: Bare URL used
null(MD034, no-bare-urls)
66-66: Bare URL used
null(MD034, no-bare-urls)
67-67: Bare URL used
null(MD034, no-bare-urls)
68-68: Bare URL used
null(MD034, no-bare-urls)
77-77: Fix grammatical error with quantifier.-NOTE: `Ledger` can't have any paths as each paths are defined by the corresponding app. +NOTE: `Ledger` can't have any paths as each path is defined by the corresponding app.🧰 Tools
🪛 LanguageTool
[grammar] ~77-~77: Use a singular noun after the quantifier ‘each’, or change it to “all”.
Context: ... NOTE:Ledgercan't have any paths as each paths are defined by the corresponding app. ...(EACH_EVERY_NNS)
📜 Review details
Configuration used: .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/hw-wallets/README.md(1 hunks)packages/hw-wallets/package.json(2 hunks)packages/hw-wallets/src/configs.ts(2 hunks)packages/hw-wallets/src/index.ts(1 hunks)packages/hw-wallets/src/ledger/ethereum/index.ts(3 hunks)packages/hw-wallets/src/trezor/ethereum/configs.ts(2 hunks)packages/hw-wallets/src/trezor/ethereum/index.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hw-wallets/src/trezor/ethereum/index.ts (2)
packages/hw-wallets/src/index.ts (1)
HWWalletProvider(121-121)packages/hw-wallets/src/trezor/ethereum/configs.ts (1)
networkBasedSupportedPaths(66-66)
🪛 GitHub Actions: Test all swap providers
packages/hw-wallets/src/ledger/ethereum/index.ts
[error] 45-45: TypeScript error TS2322: Type 'import("/home/runner/work/enKrypt/enKrypt/node_modules/@ledgerhq/hw-transport-web-ble/node_modules/@ledgerhq/hw-transport/lib/Transport").default' is not assignable to type 'import("/home/runner/work/enKrypt/enKrypt/node_modules/@ledgerhq/hw-transport/lib/Transport").default'.
🪛 GitHub Actions: Test all workspaces
packages/hw-wallets/src/ledger/ethereum/index.ts
[error] 45-45: TypeScript error TS2322: Type 'import("/home/runner/work/enKrypt/enKrypt/node_modules/@ledgerhq/hw-transport-web-ble/node_modules/@ledgerhq/hw-transport/lib/Transport").default' is not assignable to type 'import("/home/runner/work/enKrypt/enKrypt/node_modules/@ledgerhq/hw-transport/lib/Transport").default'.
🪛 GitHub Actions: Build all and Check VirusTotal
packages/hw-wallets/src/ledger/ethereum/index.ts
[error] 45-45: TypeScript error TS2322: Type 'import("/home/node_modules/@ledgerhq/hw-transport-web-ble/node_modules/@ledgerhq/hw-transport/lib/Transport").default' is not assignable to type 'import("/home/node_modules/@ledgerhq/hw-transport/lib/Transport").default'. Types of property 'tracer' are incompatible due to separate declarations of a private property 'type'.
🪛 LanguageTool
packages/hw-wallets/README.md
[uncategorized] ~29-~29: A determiner appears to be missing. Consider inserting it.
Context: ...s, passing network names and providers. Class automatically handles which wallet to u...
(AI_EN_LECTOR_MISSING_DETERMINER)
[style] ~35-~35: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...esponse> Returns wallet address based off of the path provided in thegetAddressReq...
(EN_WORDINESS_PREMIUM_OFF_OF)
[uncategorized] ~63-~63: Loose punctuation mark.
Context: ...t connections ### Types NetworkNames: https://github.com/enkryptcom/enKrypt/b...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~77-~77: Use a singular noun after the quantifier ‘each’, or change it to “all”.
Context: ... NOTE: Ledger can't have any paths as each paths are defined by the corresponding app. ...
(EACH_EVERY_NNS)
🪛 markdownlint-cli2 (0.17.2)
packages/hw-wallets/README.md
23-23: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
63-63: Bare URL used
null
(MD034, no-bare-urls)
64-64: Bare URL used
null
(MD034, no-bare-urls)
65-65: Bare URL used
null
(MD034, no-bare-urls)
66-66: Bare URL used
null
(MD034, no-bare-urls)
67-67: Bare URL used
null
(MD034, no-bare-urls)
68-68: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
packages/hw-wallets/src/trezor/ethereum/index.ts
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
packages/hw-wallets/src/trezor/ethereum/configs.ts (1)
5-30: LGTM: Improved naming clarityThe renaming from
supportedPathstonetworkBasedSupportedPathsbetter describes the purpose of this mapping, making the code more self-documenting.packages/hw-wallets/package.json (1)
22-22:Details
❓ Verification inconclusive
Verify Node.js 20 requirement justification
The Node.js version requirement was increased from 14.15.0 to 20. Ensure this major version bump is necessary and that all deployment environments support Node.js 20.
🌐 Web query:
What are the major breaking changes between Node.js 14 and Node.js 20 that might affect this hardware wallet package?💡 Result:
Major Breaking Changes Between Node.js 14 and Node.js 20 Impacting Hardware Wallet Packages
Upgrading a Node.js hardware wallet package from Node.js 14 to Node.js 20 involves several key breaking changes and ecosystem shifts that developers should review carefully.
Core Breaking Changes and Deprecations
- Deprecation of url.parse() with invalid ports: In Node.js 20, using
url.parse()with invalid ports now emits a warning. While not a runtime-breaking change yet, code that relies on lenient port parsing should be updated for future compatibility before it transitions from warning to error[1].- Web Crypto API improvements: Node.js 20 enhanced the Web Crypto API for greater interoperability and strict WebIDL argument validation. Packages that use cryptography—critical for hardware wallet operations—may require updates to ensure their method calls are compatible with stricter type coercion and validation[1].
- V8 JavaScript Engine Updates: Node.js 20 features a newer V8 version (v11.3) compared to Node.js 14 (which shipped with V8 v8.4). Code relying on deprecated or internal V8 APIs, or on legacy JS syntax and behaviors, may break or throw new errors. Pay special attention to cryptographic operations and buffer handling, as subtle V8 changes can affect functionality[1].
Ecosystem Compatibility and Dependency Updates
- Third-party library support: Many dependencies, especially those interacting with native hardware or cryptographic functions, have dropped support for Node.js 14. For example, libraries like
better-sqlite3and possibly some hardware wallet bridges/drivers must be updated to recent versions that only support Node 16+ or Node 18+—sometimes solely Node 20[7]. This can trigger a cascade of required dependency and code updates.- Build Tool and Native Addon Upgrades: Native modules built for Node 14 often require re-compilation or updates for Node 20 compatibility due to ABI (Application Binary Interface) or N-API changes. Hardware wallet packages using native bindings (e.g., USB access, cryptographic hardware libraries) may encounter build failures or runtime issues unless updated[7].
Performance and Behavioral Changes
- Enhanced performance and stricter standards: Node.js 20 introduces performance enhancements, especially in event handling, timers, and the
URLimplementation. While mostly beneficial, some performance optimizations and spec-compliance changes could alter timing or event behavior, which may surface as subtle bugs in packages with tight device communication loops[1][3].New Core Features to Consider
- Permissions Model: Node.js 20 introduced a permissions model for restricting access to system resources. If your hardware wallet package spawns processes, accesses the filesystem, or interacts with devices, the permission system might require explicit configuration or user interaction to function as before[1][3].
- Stable Test Runner: Built-in test runner is now stable, although this is less likely to affect production packages unless you rely on core testing tools[3].
Typical Migration Pain Points
- Legacy code and technical debt: Teams upgrading from Node 14 may face a significant backlog of minor and major dependency updates, further complicated by cascading incompatibilities with legacy APIs or unmaintained libraries[2][4][7].
- Production risk and testing: Any hardware wallet solution must be thoroughly regression-tested after upgrading Node.js, especially for functions dealing with private key operations, device connectivity, and transaction signing[4].
Summary Table: Key Breaking Changes
Area Node.js 14 Status Node.js 20 Breaking Changes Impact url.parse() Lenient Warning for invalid ports Web Crypto API Less strict, less interoperable Stricter validation, closer to browsers V8 Engine Older Upgraded, potential syntax/behavior differences Third-party dependency compatibility Supported by many packages Many libraries drop Node 14 support Native Addons ABI for Node 14 Rebuild/upgrade needed for Node 20 Permissions Model Not present May require code/config changes Recommendations
- Audit all dependencies for Node.js 20 support, paying particular attention to native modules and cryptography-related packages.
- Refactor code to use updated APIs (e.g., replace deprecated URL parsing methods, ensure Web Crypto method compatibility).
- Test all device interactions thoroughly under Node 20, especially for edge cases in hardware communication.
- Prepare for further changes as Node progresses; staying current will ease future upgrades[4][7].
These changes will be especially relevant to hardware wallet packages that require strong cryptography, reliable device communication, and may use legacy Node APIs. Properly planning for and testing against these breaking changes will ensure a successful and secure migration.
Citations:
- 1: https://www.itmagination.com/blog/introducing-node-js-20-new-features-updates-and-improvements
- 2: https://dev.to/meatboy/how-to-update-a-few-years-old-outdated-project-in-node-and-react-31jm
- 3: https://nodejs.org/en/blog/announcements/v20-release-announce
- 4: https://www.herodevs.com/blog-posts/why-frequent-node-js-updates-are-leaving-developers-stuck--and-how-to-break-the-cycle
- 5: https://nodejs.org/en/about/previous-releases
- 6: https://www.couchbase.com/blog/developing-bitcoin-cryptocurrency-application-nodejs-nosql/
- 7: knex/knex#5631
- 8: http://nodesource.com/blog/Update-nodejs-versions-on-windows/
- 9: https://community.umbrel.com/t/ledger-hardware-wallet/1056
- 10: https://deezer.io/node-js-20-upgrade-a-journey-through-unexpected-heap-issues-with-kubernetes-27ae3d325646
Confirm Node.js ≥20 Upgrade and Migration Plan
Upgrading from Node.js 14.x to 20.x introduces several breaking changes that can affect hardware-wallet packages. Before merging, please ensure you’ve:
• Audited all cryptography-related code and dependencies for Node 20 support
• Rebuilt and tested any native addons (USB, BLE, native crypto bindings) under Node 20’s ABI
• Updated or replaced deprecated URL parsing (url.parse()) to avoid future errors
• Verified Web Crypto calls still pass Node 20’s stricter WebIDL validation
• Examined potential behavior shifts from V8 v8.4→v11.3 (buffer handling, internal APIs)
• Confirmed your CI/CD pipelines, Docker images, and production environments can run Node 20
• Run full regression tests on device communication loops and transaction-signing flowspackages/hw-wallets/src/index.ts (1)
118-126: Excellent: Improved TypeScript API surfaceAdding explicit type exports is a great improvement that enhances the library's TypeScript developer experience. This allows consumers to import and use these types for better type safety when integrating with the hardware wallet functionality.
packages/hw-wallets/src/configs.ts (1)
64-68: LGTM: Rootstock Testnet path follows BIP44 standard.The addition of Rootstock Testnet with coin type 37310 follows the established pattern and uses the correct BIP44 path structure.
packages/hw-wallets/src/trezor/ethereum/index.ts (5)
15-15: LGTM: Import updated to match config changes.The import change from
supportedPathstonetworkBasedSupportedPathsaligns with the restructuring in the configs module.
22-22: LGTM: Extension detection property added.The
isExtensionboolean property provides a clean way to handle environment-specific behavior.
38-39: LGTM: Environment-aware network validation.The conditional network validation that only applies in extension environments is a good design pattern, allowing more flexibility in non-extension contexts.
66-66: LGTM: Consistent environment-aware path support.The conditional return of supported paths maintains consistency with the network validation logic.
137-137: LGTM: Updated to use new config structure.The static method correctly uses the updated
networkBasedSupportedPathsimport.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/hw-wallets/README.md (5)
23-27: Specify language for fenced code block
The code snippet lacks a language identifier. Addts(orjs) after the opening backticks to enable syntax highlighting.-``` +```ts import HWwalletManager from @enkryptcom/hw-wallets const hwManager = new HWwalletManager() -``` +```ts🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
23-23: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
29-29: Add missing determiner for clarity
Insert “the” before “class” to improve readability.-2. Call methods within class, passing network names and providers. +2. Call methods within the class, passing network names and providers.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: A determiner appears to be missing. Consider inserting it.
Context: ...s, passing network names and providers. Class automatically handles which wallet to u...(AI_EN_LECTOR_MISSING_DETERMINER)
35-35: Simplify wording
Replace “based off of” with “based on” for conciseness.-Returns wallet address based off of the path provided in the `getAddressRequest`. +Returns wallet address based on the path provided in the `getAddressRequest`.🧰 Tools
🪛 LanguageTool
[style] ~35-~35: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...esponse>Returns wallet address based off of the path provided in thegetAddressReq...(EN_WORDINESS_PREMIUM_OFF_OF)
63-68: Convert bare URLs into markdown links
Wrap or replace bare URLs with link text to improve readability and accessibility.-`NetworkNames`: https://github.com/enkryptcom/enKrypt/blob/main/packages/types/src/networks.ts#L1 +`NetworkNames`: [NetworkNames type](https://github.com/enkryptcom/enKrypt/blob/main/packages/types/src/networks.ts#L1) -`getAddressRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L74 +`getAddressRequest`: [getAddressRequest type](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L74) -`AddressResponse`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L31 +`AddressResponse`: [AddressResponse type](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L31) -`SignMessageRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L54 +`SignMessageRequest`: [SignMessageRequest type](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L54) -`SignTransactionRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L65 +`SignTransactionRequest`: [SignTransactionRequest type](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L65) -`isConnectedRequest`: https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L78 +`isConnectedRequest`: [isConnectedRequest type](https://github.com/enkryptcom/enKrypt/blob/main/packages/hw-wallets/src/types.ts#L78)🧰 Tools
🪛 LanguageTool
[uncategorized] ~63-~63: Loose punctuation mark.
Context: ...t connections ### TypesNetworkNames: https://github.com/enkryptcom/enKrypt/b...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
63-63: Bare URL used
null(MD034, no-bare-urls)
64-64: Bare URL used
null(MD034, no-bare-urls)
65-65: Bare URL used
null(MD034, no-bare-urls)
66-66: Bare URL used
null(MD034, no-bare-urls)
67-67: Bare URL used
null(MD034, no-bare-urls)
68-68: Bare URL used
null(MD034, no-bare-urls)
80-80: Fix pluralization and agreement
Use singular “path” and verb agreement.-`Ledger` can't have any paths as each paths are defined by the corresponding app. +`Ledger` can't have any paths as each path is defined by the corresponding app.🧰 Tools
🪛 LanguageTool
[grammar] ~80-~80: Use a singular noun after the quantifier ‘each’, or change it to “all”.
Context: ...est.Ledgercan't have any paths as each paths are defined by the corresponding app. ...(EACH_EVERY_NNS)
[uncategorized] ~80-~80: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ger` can't have any paths as each paths are defined by the corresponding app. ####...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/hw-wallets/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/hw-wallets/README.md
[uncategorized] ~29-~29: A determiner appears to be missing. Consider inserting it.
Context: ...s, passing network names and providers. Class automatically handles which wallet to u...
(AI_EN_LECTOR_MISSING_DETERMINER)
[style] ~35-~35: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...esponse> Returns wallet address based off of the path provided in thegetAddressReq...
(EN_WORDINESS_PREMIUM_OFF_OF)
[uncategorized] ~63-~63: Loose punctuation mark.
Context: ...t connections ### Types NetworkNames: https://github.com/enkryptcom/enKrypt/b...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~80-~80: Use a singular noun after the quantifier ‘each’, or change it to “all”.
Context: ...est. Ledger can't have any paths as each paths are defined by the corresponding app. ...
(EACH_EVERY_NNS)
[uncategorized] ~80-~80: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ger` can't have any paths as each paths are defined by the corresponding app. ####...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
packages/hw-wallets/README.md
23-23: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
63-63: Bare URL used
null
(MD034, no-bare-urls)
64-64: Bare URL used
null
(MD034, no-bare-urls)
65-65: Bare URL used
null
(MD034, no-bare-urls)
66-66: Bare URL used
null
(MD034, no-bare-urls)
67-67: Bare URL used
null
(MD034, no-bare-urls)
68-68: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/hw-wallets/src/configs.ts (1)
65-69: Non-standard BIP44 coin type for Rootstock Testnet.The rootstockTestnet configuration uses coin type 37310, which is not listed in the official SLIP-0044 registry. This issue has already been flagged in a previous review.
🧹 Nitpick comments (1)
packages/hw-wallets/src/index.ts (1)
118-118: Drop the debugconsole.logThe
console.log(network, 'AAAAAA')looks like leftover debugging; letting it ship will spam logs whenever no provider matches. Please remove it.Apply this diff:
- console.log(network, 'AAAAAA')
📜 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/hw-wallets/package.json(3 hunks)packages/hw-wallets/src/configs.ts(4 hunks)packages/hw-wallets/src/index.ts(2 hunks)packages/hw-wallets/src/ledger/bitcoin/configs.ts(1 hunks)packages/hw-wallets/src/ledger/bitcoin/index.ts(3 hunks)packages/hw-wallets/src/ledger/ethereum/index.ts(3 hunks)packages/hw-wallets/src/trezor/ethereum/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hw-wallets/src/ledger/ethereum/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/hw-wallets/src/trezor/ethereum/index.ts (2)
packages/hw-wallets/src/index.ts (1)
HWWalletProvider(130-130)packages/hw-wallets/src/trezor/ethereum/configs.ts (1)
networkBasedSupportedPaths(66-66)
⏰ 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: buildAll
🔇 Additional comments (3)
packages/hw-wallets/src/configs.ts (3)
22-22: LGTM!The addition of BitcoinTest to ledgerAppNames is consistent with the new bitcoinTestSegwitLedger path configuration and follows the existing naming conventions.
90-94: LGTM!The bitcoinTestSegwitLedger configuration correctly uses coin type 1 (the standard testnet coin type) and follows the BIP84 derivation path for native SegWit addresses.
130-265: Verify non-standard derivation paths for Lightstreams Network and Binance Chain
- LightstreamsNetwork uses
m/44'/60'/0'/{index}(missing the/0change level)- BinanceChain uses
m/44'/714'/{index}(missing both/0'/0account/change levels)Confirm these entries match the official SLIP-0044 specs and hardware-wallet derivation requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/hw-wallets/src/configs.ts (1)
65-69: Address the non-standard BIP44 coin type flagged in previous review.The coin type 37310 for rootstockTestnet is not in the SLIP-0044 registry, as noted in a previous review. This remains unresolved.
Based on the previous review feedback, either:
- Add a comment explaining this is a custom/non-standard testnet coin type, or
- Update to use the standard testnet coin type (1) or correct SLIP-0044 value
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/hw-wallets/src/configs.ts(5 hunks)packages/hw-wallets/src/index.ts(2 hunks)packages/hw-wallets/src/trezor/bitcoin/configs.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/hw-wallets/src/trezor/bitcoin/configs.ts (1)
packages/hw-wallets/src/configs.ts (1)
bip44Paths(272-272)
⏰ 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: buildAll
🔇 Additional comments (5)
packages/hw-wallets/src/index.ts (1)
125-134: Clean API expansion with proper type exports.Exposing these types makes the library more consumable for external code. The use of
export typesyntax is appropriate for TypeScript type-only exports.packages/hw-wallets/src/configs.ts (3)
22-22: LGTM!The Bitcoin Test ledger app name is appropriately added to support testnet functionality.
90-94: LGTM!The Bitcoin testnet paths correctly use coin type 1 (standard BIP44 testnet), and the structural differences between Ledger and Trezor derivation patterns are appropriate for each hardware wallet's implementation.
Also applies to: 120-124
135-270: Comprehensive network support expansion looks good.The MyEtherWallet-derived paths follow consistent patterns and align with established derivation standards. A few observations:
- lightstreamsNetwork (line 251-255) and poaNetwork (line 136-140) share coin type 60 (Ethereum) with slight path variations, which is intentional
- binanceChain (line 266-270) uses a simpler 3-level derivation pattern (
m/44'/714'/{index}), which matches Binance's specificationThe source attribution comment is helpful for maintenance.
packages/hw-wallets/src/trezor/bitcoin/configs.ts (1)
6-6: bip44Paths.bitcoinTestSegwitTrezor exists
Confirmed in packages/hw-wallets/src/configs.ts:120.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/hw-wallets/src/trezor/trezorConnect.ts (1)
23-23: Consider using consistent appUrl across both paths.The non-extension path uses
"http://www.myetherwallet.com"while the extension path uses"https://www.enkrypt.com"(line 9). This inconsistency appears unintentional and could lead to confusion.Apply this diff to align with the extension path:
- appUrl: "http://www.myetherwallet.com", + appUrl: "https://www.enkrypt.com",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/hw-wallets/src/trezor/ethereum/index.ts(4 hunks)packages/hw-wallets/src/trezor/trezorConnect.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hw-wallets/src/trezor/ethereum/index.ts
⏰ 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: buildAll
🔇 Additional comments (1)
packages/hw-wallets/src/trezor/trezorConnect.ts (1)
19-26: LGTM! Correctly fixed the property access.The changes correctly remove the redundant
.defaultaccess. SinceTrezorConnectis already assigned to the default export at line 18, callingTrezorConnect.init()and returningTrezorConnectdirectly is the correct approach.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/hw-wallets/src/trezor/ethereum/index.ts (1)
29-31: Guard access tochrometo avoid ReferenceError.This issue was already flagged in a previous review. Accessing
chromedirectly throws a ReferenceError outside a Chrome-extension context because the identifier is not defined in Node/browser environments. Use atypeof chrome !== "undefined"guard before accessingchrome.runtime.Apply this diff:
- this.isExtension = !!( - chrome && chrome.runtime && chrome.runtime.getPlatformInfo - ); + this.isExtension = + typeof chrome !== "undefined" && !!chrome.runtime?.getPlatformInfo;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/hw-wallets/src/ledger/bitcoin/index.ts(3 hunks)packages/hw-wallets/src/trezor/ethereum/index.ts(4 hunks)packages/hw-wallets/src/trezor/trezorConnect.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hw-wallets/src/ledger/bitcoin/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/hw-wallets/src/trezor/ethereum/index.ts (2)
packages/hw-wallets/src/index.ts (1)
HWWalletProvider(129-129)packages/hw-wallets/src/trezor/ethereum/configs.ts (1)
networkBasedSupportedPaths(66-66)
⏰ 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: buildAll
🔇 Additional comments (4)
packages/hw-wallets/src/trezor/trezorConnect.ts (1)
19-19: LGTM! Clean refactoring.The removal of
.defaultaccess is correct since line 17-18 already extracts the default export intoTrezorConnect. CallingTrezorConnect.init()and returningTrezorConnectis now equivalent to the previousTrezorConnect.default.init()andTrezorConnect.defaultpattern. This simplifies the code without changing functionality.Also applies to: 26-26
packages/hw-wallets/src/trezor/ethereum/index.ts (3)
17-17: LGTM! Import aligns with configuration refactor.The rename from
supportedPathstonetworkBasedSupportedPathscorrectly reflects the shift to network-specific path configuration.
24-24: LGTM! Property declaration is correct.The
isExtensionproperty appropriately tracks whether the code is running in a Chrome extension context for conditional validation logic.
157-159: LGTM! Static method correctly extracts supported networks.The implementation properly returns all network keys from the
networkBasedSupportedPathsconfiguration object.
| if (this.isExtension && !networkBasedSupportedPaths[this.network]) | ||
| return Promise.reject(new Error("trezor-ethereum: Invalid network name")); |
There was a problem hiding this comment.
Inconsistent network validation creates undefined behavior.
The conditional validation only rejects invalid networks in extension contexts. In non-extension environments, invalid networks pass validation but cause getSupportedPaths() (line 68) to return undefined instead of PathType[], breaking the type contract and potentially crashing downstream consumers.
Either validate for all contexts:
- if (this.isExtension && !networkBasedSupportedPaths[this.network])
+ if (!networkBasedSupportedPaths[this.network])
return Promise.reject(new Error("trezor-ethereum: Invalid network name"));Or add a fallback in getSupportedPaths:
getSupportedPaths(): PathType[] {
return networkBasedSupportedPaths[this.network] || [];
}🤖 Prompt for AI Agents
In packages/hw-wallets/src/trezor/ethereum/index.ts around lines 40-41, the
current code only rejects invalid network names when running in extension mode,
causing getSupportedPaths() to sometimes return undefined and break its
PathType[] contract; fix by enforcing network validation for all contexts
(remove the isExtension guard so invalid networks always throw) or,
alternatively, update getSupportedPaths() to return a safe default array when
lookup fails (e.g., return an empty array when
networkBasedSupportedPaths[this.network] is undefined) so the function always
returns PathType[].
… issue when first time connecting
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/hw-wallets/src/ledger/bitcoin/index.ts (1)
202-205: Skipclose()when no transport was ever opened
this.transportremainsnullafter failed or skipped initialization. Invokingthis.transport.close()in that state throws (Cannot read properties of null), so cleanup explodes exactly when users need it after an init failure.Short-circuit when there is nothing to close:
close(): Promise<void> { - // eslint-disable-next-line @typescript-eslint/no-empty-function - return this.transport.close().catch(() => { }); + if (!this.transport) return Promise.resolve(); + return this.transport.close().catch(() => undefined); }
📜 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 (5)
packages/hw-wallets/src/ledger/bitcoin/index.ts(4 hunks)packages/hw-wallets/src/ledger/ethereum/index.ts(3 hunks)packages/hw-wallets/src/ledger/ledgerInitializer.ts(1 hunks)packages/hw-wallets/src/ledger/solana/index.ts(2 hunks)packages/hw-wallets/src/ledger/substrate/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/hw-wallets/src/ledger/substrate/index.ts (1)
packages/hw-wallets/src/ledger/ledgerInitializer.ts (1)
LedgerInit(5-42)
packages/hw-wallets/src/ledger/bitcoin/index.ts (1)
packages/hw-wallets/src/ledger/ledgerInitializer.ts (1)
LedgerInit(5-42)
packages/hw-wallets/src/ledger/solana/index.ts (1)
packages/hw-wallets/src/ledger/ledgerInitializer.ts (1)
LedgerInit(5-42)
packages/hw-wallets/src/ledger/ethereum/index.ts (1)
packages/hw-wallets/src/ledger/ledgerInitializer.ts (1)
LedgerInit(5-42)
⏰ 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). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
| close(): Promise<void> { | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| return this.transport.close().catch(() => {}); | ||
| return this.transport.close().catch(() => { }); | ||
| } |
There was a problem hiding this comment.
Prevent close() from crashing after failed init
If transport setup fails and this.transport stays null, calling close() triggers a TypeError before the catch runs. That makes cleanup impossible after an init failure.
Guard the call (same pattern as other Ledger classes):
close(): Promise<void> {
- // eslint-disable-next-line @typescript-eslint/no-empty-function
- return this.transport.close().catch(() => { });
+ if (!this.transport) return Promise.resolve();
+ return this.transport.close().catch(() => undefined);
}📝 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.
| close(): Promise<void> { | |
| // eslint-disable-next-line @typescript-eslint/no-empty-function | |
| return this.transport.close().catch(() => {}); | |
| return this.transport.close().catch(() => { }); | |
| } | |
| close(): Promise<void> { | |
| if (!this.transport) return Promise.resolve(); | |
| return this.transport.close().catch(() => undefined); | |
| } |
🤖 Prompt for AI Agents
In packages/hw-wallets/src/ledger/ethereum/index.ts around lines 155 to 158,
calling this.transport.close() can throw a TypeError when this.transport is null
after a failed init; update close() to first check if this.transport is non-null
and only call close().catch(...) when present, otherwise return a resolved
Promise<void> (matching the guard pattern used in other Ledger classes) so
cleanup never crashes when transport was not created.
| async init(): Promise<boolean> { | ||
|
|
||
| if (!this.transport) { | ||
| try { | ||
| const BLEsupport = await bleTransport.isSupported(); | ||
| if (BLEsupport) { | ||
| const transport = await bleTransport.create(); | ||
| transport.on("disconnect", () => { // connection wasnt succesful | ||
| this.transport = null; | ||
| }) | ||
| this.transport = transport; | ||
| } | ||
|
|
||
| } catch { | ||
| const support = await webUsbTransport.isSupported(); | ||
| if (support) { | ||
| this.transport = await webUsbTransport.openConnected().then((res) => { | ||
| if (!res) return webUsbTransport.create(); | ||
| return res; | ||
| }); | ||
|
|
||
| } else { | ||
| return Promise.reject( | ||
| new Error("ledger-ethereum: webusb is not supported"), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Always attempt WebUSB when BLE is unavailable
When bleTransport.isSupported() resolves false (the common case on desktops), the try block exits without throwing, so the catch never runs. We skip the WebUSB path entirely, leave this.transport as null, yet still resolve true. Every subsequent Ledger call then fails because no transport exists.
Move the WebUSB fallback out of the catch so it runs whenever BLE isn’t both supported and successfully created:
async init(): Promise<boolean> {
-
- if (!this.transport) {
- try {
- const BLEsupport = await bleTransport.isSupported();
- if (BLEsupport) {
- const transport = await bleTransport.create();
- transport.on("disconnect", () => { // connection wasnt succesful
- this.transport = null;
- })
- this.transport = transport;
- }
-
- } catch {
- const support = await webUsbTransport.isSupported();
- if (support) {
- this.transport = await webUsbTransport.openConnected().then((res) => {
- if (!res) return webUsbTransport.create();
- return res;
- });
-
- } else {
- return Promise.reject(
- new Error("ledger-ethereum: webusb is not supported"),
- );
- }
- }
-
- }
+ if (this.transport) return true;
+
+ let bleError: unknown = null;
+ try {
+ if (await bleTransport.isSupported()) {
+ const transport = await bleTransport.create();
+ transport.on("disconnect", () => {
+ this.transport = null;
+ });
+ this.transport = transport;
+ return true;
+ }
+ } catch (err) {
+ bleError = err;
+ }
+
+ const usbSupported = await webUsbTransport.isSupported();
+ if (!usbSupported) {
+ const reason = bleError instanceof Error ? ` (BLE failed: ${bleError.message})` : "";
+ return Promise.reject(
+ new Error(`ledger: no supported transport available${reason}`),
+ );
+ }
+ this.transport = await webUsbTransport.openConnected().then((res) => {
+ if (!res) return webUsbTransport.create();
+ return res;
+ });
return true;
}(Feel free to adjust messaging, but ensure the fallback executes when BLE isn’t supported.)
📝 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.
| async init(): Promise<boolean> { | |
| if (!this.transport) { | |
| try { | |
| const BLEsupport = await bleTransport.isSupported(); | |
| if (BLEsupport) { | |
| const transport = await bleTransport.create(); | |
| transport.on("disconnect", () => { // connection wasnt succesful | |
| this.transport = null; | |
| }) | |
| this.transport = transport; | |
| } | |
| } catch { | |
| const support = await webUsbTransport.isSupported(); | |
| if (support) { | |
| this.transport = await webUsbTransport.openConnected().then((res) => { | |
| if (!res) return webUsbTransport.create(); | |
| return res; | |
| }); | |
| } else { | |
| return Promise.reject( | |
| new Error("ledger-ethereum: webusb is not supported"), | |
| ); | |
| } | |
| } | |
| } | |
| return true; | |
| } | |
| async init(): Promise<boolean> { | |
| // If we already have a transport, no need to re-initialize | |
| if (this.transport) return true; | |
| let bleError: unknown = null; | |
| try { | |
| // Try BLE first if supported | |
| if (await bleTransport.isSupported()) { | |
| const transport = await bleTransport.create(); | |
| transport.on("disconnect", () => { | |
| this.transport = null; | |
| }); | |
| this.transport = transport; | |
| return true; | |
| } | |
| } catch (err) { | |
| // Remember why BLE failed, but fall through to USB | |
| bleError = err; | |
| } | |
| // Fallback to WebUSB | |
| const usbSupported = await webUsbTransport.isSupported(); | |
| if (!usbSupported) { | |
| const reason = bleError instanceof Error ? ` (BLE failed: ${bleError.message})` : ""; | |
| return Promise.reject( | |
| new Error(`ledger: no supported transport available${reason}`), | |
| ); | |
| } | |
| this.transport = await webUsbTransport.openConnected().then(res => { | |
| if (!res) return webUsbTransport.create(); | |
| return res; | |
| }); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In packages/hw-wallets/src/ledger/ledgerInitializer.ts around lines 11 to 41,
the current logic only tries the WebUSB fallback inside the catch block so when
bleTransport.isSupported() resolves false the catch is never hit and WebUSB is
skipped; change the flow so that after awaiting bleTransport.isSupported() you
explicitly branch: if BLE is supported attempt to create BLE transport and set
this.transport (attach a disconnect handler) catching only errors from creation;
if BLE is not supported or BLE creation fails then check
webUsbTransport.isSupported(), and if supported openConnected()/create and
assign this.transport (attach disconnect handler), otherwise reject with the
existing WebUSB not supported error; ensure the function still returns true on
success and rejects on complete failure.
| close(): Promise<void> { | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| return this.transport.close().catch(() => {}); | ||
| return this.transport.close().catch(() => { }); | ||
| } |
There was a problem hiding this comment.
Handle the no-transport case before calling close()
When initialization never bound a transport (e.g., both BLE and WebUSB unsupported), this.transport is still null. Invoking .close() then throws immediately, so callers can’t safely clean up.
Add a simple guard:
close(): Promise<void> {
- // eslint-disable-next-line @typescript-eslint/no-empty-function
- return this.transport.close().catch(() => { });
+ if (!this.transport) return Promise.resolve();
+ return this.transport.close().catch(() => undefined);
}📝 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.
| close(): Promise<void> { | |
| // eslint-disable-next-line @typescript-eslint/no-empty-function | |
| return this.transport.close().catch(() => {}); | |
| return this.transport.close().catch(() => { }); | |
| } | |
| close(): Promise<void> { | |
| if (!this.transport) return Promise.resolve(); | |
| return this.transport.close().catch(() => undefined); | |
| } |
🤖 Prompt for AI Agents
In packages/hw-wallets/src/ledger/solana/index.ts around lines 77 to 80, the
close() method calls this.transport.close() without checking whether
this.transport is null, causing an immediate throw when no transport was
initialized; update close() to guard for a missing transport by returning a
resolved Promise if this.transport is null/undefined, otherwise call
this.transport.close() and propagate/catch its Promise as before so callers can
safely clean up even when no transport was created.
| close(): Promise<void> { | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| return this.transport.close().catch(() => {}); | ||
| return this.transport.close().catch(() => { }); | ||
| } |
There was a problem hiding this comment.
Guard close() when no transport exists
this.transport stays null whenever initialization fails or was never run. Calling this.transport.close() in that state throws before the catch executes, so consumers hit a runtime error when attempting to clean up after a failed init.
Use a guard (or optional chaining) so close() resolves immediately if there is no transport:
close(): Promise<void> {
- // eslint-disable-next-line @typescript-eslint/no-empty-function
- return this.transport.close().catch(() => { });
+ if (!this.transport) return Promise.resolve();
+ return this.transport.close().catch(() => undefined);
}🤖 Prompt for AI Agents
In packages/hw-wallets/src/ledger/substrate/index.ts around lines 66 to 69,
close() currently calls this.transport.close() unguarded which throws when
this.transport is null; change it to first check for a transport (e.g., if
(!this.transport) return Promise.resolve(); or use optional chaining like return
this.transport?.close().catch(() => {});) so the method resolves immediately
when no transport exists and still catches errors from close().
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension/src/libs/keyring/public-keyring.ts(1 hunks)
⏰ 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). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
| allKeys[ | ||
| 'bc1qwgpekhhfekclmp58kyzmlsdzey948d4395nvs8' | ||
| ] = { | ||
| address: | ||
| 'bc1qwgpekhhfekclmp58kyzmlsdzey948d4395nvs8', | ||
| basePath: "m/49'/2'/0'/1", | ||
| name: 'fake btc account #1', | ||
| pathIndex: 0, | ||
| publicKey: '0x0', | ||
| signerType: SignerType.secp256k1btc, | ||
| walletType: WalletType.mnemonic, | ||
| isHardware: false, | ||
| isTestWallet: true, | ||
| }; |
There was a problem hiding this comment.
Verify the test account's basePath matches the intended test scenario.
The basePath "m/49'/2'/0'/1" appears inconsistent with the account details:
- BIP49 (
m/49') is typically for P2SH-wrapped SegWit addresses (starting with3), not native SegWit - Coin type
2'per SLIP-44 is Litecoin testnet, not Bitcoin - The
bc1qaddress prefix indicates Bitcoin native SegWit, which typically uses BIP84 (m/84') with coin type0'
Since this is test data, please confirm this path matches your intended test scenario for Bitcoin accounts.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
packages/hw-wallets/package.json(3 hunks)
⏰ 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). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (2)
packages/hw-wallets/package.json (2)
8-14: Exports field correctly follows modern package.json conventions.The new exports field properly maps TypeScript types, ESM import, and CommonJS require entry points, enabling better tree-shaking and dual-module support. This aligns well with the publishConfig section.
66-66: No action needed. The version constraints are functioning correctly without conflicts.The @ledgerhq/hw-transport-web-ble@^6.29.13 and @ledgerhq/hw-transport-webusb@^6.29.13 packages have no declared peer dependencies, eliminating the risk of peer dependency conflicts. The yarn.lock file resolves all versions cleanly without duplicates or conflicts. TypeScript imports work without issues across the codebase (ledgerInitializer.ts, substrateApps.ts, and ethereum/index.ts). The recent "fix: conflicts" commit confirms any version management concerns were already addressed. The version constraint difference (6.29.13 vs 6.31.13) reflects standard transitive dependency resolution within compatible caret ranges and poses no compatibility issues.
Likely an incorrect or invalid review comment.
| "engines": { | ||
| "node": ">=14.15.0" | ||
| "node": ">=20" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Node 20 compatibility and transport package versions
echo "=== Checking package.json for transport dependencies ==="
cat packages/hw-wallets/package.json | grep -A 20 '"dependencies"'
echo -e "\n=== Checking for Node version config files ==="
cd packages/hw-wallets
for file in .nvmrc .node-version; do
if [ -f "$file" ]; then
echo "Found $file:"; cat "$file"
fi
done
cd ../..
echo -e "\n=== Checking root .nvmrc or node version config ==="
for file in .nvmrc .node-version; do
if [ -f "$file" ]; then
echo "Found $file:"; cat "$file"
fi
done
echo -e "\n=== Checking for CHANGELOG or migration docs ==="
fd -i changelog -i migration -i "release.*notes" packages/hw-wallets/ || echo "No changelog found in hw-wallets"
echo -e "\n=== Checking if Node version requirement is documented in README ==="
if [ -f "packages/hw-wallets/README.md" ]; then
echo "Checking README.md:"; grep -i "node" packages/hw-wallets/README.md || echo "No Node version mentioned"
fi
echo -e "\n=== Checking git for any Node version changes ==="
git log --oneline --all -S ">=20" -- "packages/hw-wallets/package.json" 2>/dev/null | head -5 || echo "Git history not available"
echo -e "\n=== Checking for any CI/CD config mentioning Node version ==="
fd "\.github|\.gitlab|\.circleci|\.travis" -t d || echo "No CI configs found"Repository: enkryptcom/enKrypt
Length of output: 1532
🏁 Script executed:
#!/bin/bash
# Check if hw-wallets .nvmrc is the issue and confirm package version compatibility
echo "=== Checking Node versions represented by .nvmrc files ==="
echo "lts/fermium = Node 14 (EOL)"
echo "v22.18.0 = Node 22 LTS"
echo -e "\n=== Verifying hw-wallets/.nvmrc content ==="
cat packages/hw-wallets/.nvmrc
echo -e "\n=== Checking @ledgerhq peer dependencies in lock file ==="
if [ -f "pnpm-lock.yaml" ]; then
echo "Checking pnpm-lock.yaml for @ledgerhq transport conflicts..."
rg -A 3 "@ledgerhq/hw-transport" pnpm-lock.yaml | head -40
elif [ -f "package-lock.json" ]; then
echo "Checking package-lock.json..."
grep -A 5 "@ledgerhq/hw-transport" package-lock.json | head -30
fi
echo -e "\n=== Checking if transport package versions have peer dependency constraints ==="
# Search for package metadata that might indicate version conflicts
rg "peerDependencies|engines" packages/hw-wallets/package.jsonRepository: enkryptcom/enKrypt
Length of output: 382
Update hw-wallets/.nvmrc to align with Node >=20 requirement.
The package.json correctly specifies engines.node: ">=20" and this is documented in the README. However, packages/hw-wallets/.nvmrc contains lts/fermium (Node 14), which creates a developer experience issue—tools like nvm will override to Node 14 despite the package requiring Node >=20. Update this file to v20 or higher to match the enforced constraint.
🤖 Prompt for AI Agents
In packages/hw-wallets/.nvmrc (related to package.json lines 28-30) the .nvmrc
currently pins lts/fermium (Node 14) which conflicts with package.json's
engines.node ">=20"; update packages/hw-wallets/.nvmrc to "v20" (or a newer vX
where X>=20) so tools like nvm will use Node 20+, ensuring it matches the
enforced constraint and README.
Summary by CodeRabbit
New Features
Documentation
Chores
Refactor
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.