-
Notifications
You must be signed in to change notification settings - Fork 90
feat: gridplus cleanup #741
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
Conversation
Complete GridPlus Lattice integration with EIP-1559 support, EIP-712 signing, on-device calldata decoding, and connection persistence. Features: - Full EIP-1559 transaction support with proper yParity handling - EIP-712 typed data signing with general signing mode - On-device calldata decoding for clear signing (FOX, RFOX contracts) - Connection persistence via privKey for seamless reconnection - Request deduplication and caching for optimal performance - Proper transaction construction using @ethereumjs/tx 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Enable Solana address derivation by setting support flags to true. Solana implementation uses ED25519_PUB flag with path m/44'/501'/account'/0'. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Check that Lattice firmware >= 0.14.0 before attempting Solana address derivation with ED25519 flag. Add console log to help debug firmware version issues. Also add iterIdx: 0 parameter explicitly to getAddresses call. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Switch from direct client.getAddresses() to SDK's fetchSolanaAddresses() wrapper which requires proper global state management. Initialize SDK state in transport setup with localStorage callbacks for proper client persistence. Changes: - Add SDK state initialization (setLoadClient/setSaveClient) in transport.ts - Use fetchSolanaAddresses() wrapper instead of direct client calls - Add localStorage getters/setters for client state persistence - Fetch 10 addresses at once for better performance (SDK default) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add bs58 dependency for Solana address encoding - SDK fetchSolanaAddresses returns Buffer objects - Convert 32-byte public key buffers to base58 addresses 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Cache all 10 addresses returned from fetchSolanaAddresses - Check cache before fetching to avoid redundant requests - Reduces device queries for sequential account indices 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add Bitcoin support (Legacy, SegWit, Wrapped SegWit) - Add Dogecoin support (Legacy only) - Implement Solana signing with SDK wrapper - Implement Bitcoin/Dogecoin signing with SDK wrappers - Batch address caching for all chains (10 addresses at once) - Support flags for BTC and BTCInfo 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add CosmosWallet and ThorchainWallet interface implementation - Implement cosmosGetAddress with bech32 encoding (using secp256k1 pubkey) - Implement thorchainGetAddress with bech32 encoding (thor/tthor prefix) - Add CosmosWalletInfo and ThorchainWalletInfo support - Add dependencies: bech32, crypto-js for address derivation - Cosmos and THORChain signing methods stubbed (to be implemented) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Downgrade bech32 from ^2.0.0 to ^1.1.4 (match hdwallet-native) - Remove @types/bech32 devDependency (not needed) - Fixes TypeScript build errors caused by version mismatch 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add getPublicKeys() method to retrieve xpubs for account discovery: - Support secp256k1 curve (Bitcoin, Dogecoin) using SECP256K1_XPUB flag - Support ed25519 curve (Solana) using ED25519_PUB flag - Uses fetchAddressesByDerivationPath() from GridPlus SDK - Returns array of PublicKey objects with xpub strings - Error handling with null fallback for failed requests This fixes the "GridPlus public key retrieval not implemented yet" error in UTXO chain adapters for account discovery. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix getPublicKeys() path parsing (remove "m/" prefix for GridPlus SDK) - Implement cosmosSignTx() using GridPlus general signing with secp256k1 - Implement thorchainSignTx() using GridPlus general signing with secp256k1 - Add required dependencies: @cosmjs/amino, @cosmjs/stargate, proto-tx-builder, p-lazy, bs58check - Create signer adapter for proto-tx-builder integration - Sign Amino documents with SHA256 hash using GridPlus device This enables Cosmos and THORChain transaction signing on GridPlus Lattice. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…rt isGridPlus - Fix cosmosGetAddress() to strip 'm/' prefix from path - Fix thorchainGetAddress() to strip 'm/' prefix from path - Export isGridPlus() helper function - Add comprehensive debug logging to getPublicKeys() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Refactor ethGetAddress to use fetchAddresses with batch caching (fetch 10 at once) - Add convertXpubVersion utility to convert xpub→dgub for Dogecoin - Fix Solana cache key format to match msg.addressNList - Fix Cosmos/THORChain Buffer→hex conversion for createCosmosAddress 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix cosmosGetAddress and thorchainGetAddress to use correct BIP32 paths * Extract address index from last path element * Pass base path (without index) to fetchAddressesByDerivationPath * This fixes wrong address derivation for GridPlus Cosmos/THORChain - Add complete MAYAchain support to GridPlus wallet * Implement mayachainGetAddress() and mayachainSignTx() * Add mayachainGetAccountPaths() and mayachainNextAccountPath() * MAYAchain uses slip44=931 and "maya" bech32 prefix 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add setupWithoutConnect() method to GridPlusTransport that skips client.connect() call when reconnecting with existing privKey, avoiding unnecessary pairing screen on device. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Track full pairing flow including: - Transport creation and reuse - Client initialization and SDK setup - connect() and pair() calls with errors - Wallet creation steps Debugging ephemeralPub error and dead click issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When client.connect() fails with "Device Locked", the SDK client is left in an invalid state without ephemeral keys. This causes pair() to fail with ephemeralPub error. Retry connect() before pair() to fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove duplicate Client creation that caused ephemeralPub errors. Now properly using SDK's setup() + getClient() workflow instead of creating both SDK client AND our own client instance. Fixes: - ephemeralPub error on initial pairing - Device Locked error handling - Reconnection flow with SDK's stored state 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…P44 paths GridPlus was incorrectly deriving Cosmos SDK chains (ATOM, THOR, MAYA) from 4-level paths instead of the standard 5-level BIP44 paths. Root cause: Code was removing the last element from addressNList before calling fetchAddressesByDerivationPath() with startPathIndex. However, the SDK function ignores startPathIndex when no wildcard is present in the path, resulting in derivation from the wrong (truncated) path. Example bug for m/44'/118'/0'/0/0: - Before: Derived from m/44'/118'/0'/0 (WRONG) - After: Derives from m/44'/118'/0'/0/0 (CORRECT) Fixed functions: - cosmosGetAddress, cosmosSignTx - thorchainGetAddress, thorchainSignTx - mayachainGetAddress, mayachainSignTx This matches the derivation used by Native, KeepKey, and other wallets. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
GridPlus SDK returns uncompressed 65-byte secp256k1 pubkeys, but Cosmos SDK chains (ATOM, THOR, MAYA) require compressed 33-byte pubkeys for address derivation. Without compression, completely different addresses are generated. Changes: - Add @bitcoinerlab/secp256k1 dependency to hdwallet-gridplus - Compress pubkeys in cosmosGetAddress, thorchainGetAddress, mayachainGetAddress - Compress pubkeys in cosmosSignTx, thorchainSignTx, mayachainSignTx - Add migration 6 in web to clear Cosmos SDK AccountIds for re-derivation This fixes wrong address generation for Cosmos SDK chains when using GridPlus. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/hdwallet-gridplus/src/bitcoin.ts (3)
45-61: Critical: Missing return statement causes addressNList-based outputs to fail.When
spendOutput.addressNListis provided, the code derives an address viabtcGetAddressand validates it's not null, but never returns it. Execution unconditionally falls through to line 60, throwing an error even for valid addressNList outputs.Apply this diff to fix:
if (spendOutput.addressNList) { const address = await btcGetAddress(client, { addressNList: spendOutput.addressNList, coin: msg.coin, scriptType: spendOutput.scriptType as unknown as core.BTCInputScriptType, }); if (!address) throw new Error("No address returned from device"); - return address + return address; } throw new Error("Invalid output (no address or addressNList specified).");
179-180: Update misleading comment —singleHash: trueis the correct GridPlus integration.The comment "pending gridplus fix for hashType.NONE" misrepresents
singleHash: trueas a temporary workaround. GridPlus's signing API requires specifying the hash type and returns raw ECDSA r||s signatures, which matches bitcoinjs-lib's SignerAsync expectation of 64-byte r||s. Single-hash mode is the required integration pattern, not a pending fix.Update the comment to clarify the design:
- // single sha256 hash and allow device to perform second sha256 hash when signing (pending gridplus fix for hashType.NONE) + // GridPlus signing requires single SHA-256 input and returns raw 64-byte r||s signatures compatible with bitcoinjs-lib SignerAsync await psbt.signInputAsync(i, signer, undefined, true);
183-197: Add THORChain vout ordering validation before returning.The GridPlus Bitcoin implementation is missing THORChain vout ordering validation present in all other wallet implementations. When
msg.vaultAddressis set, validate usingcore.validateVoutOrdering(msg, tx)before returning, throwing an error if ordering is improper.Add validation before the return statement:
const tx = psbt.extractTransaction(true); + if (msg.vaultAddress && !core.validateVoutOrdering(msg, tx)) { + throw new Error("THORChain vault transaction outputs are not properly ordered"); + } + const signatures = tx.ins.map((input) => { if (input.witness.length > 0) { return Buffer.from(input.witness[0]).toString("hex"); } else { const sigLen = input.script[0]; return Buffer.from(input.script.slice(1, sigLen)).toString("hex"); } }); return { signatures, serializedTx: tx.toHex() };packages/hdwallet-gridplus/src/gridplus.ts (1)
308-318: Lifecycle methods are silent no-ops — prefer explicit unsupported errors.These methods (
cancel,clearSession,initialize,loadDevice,recover,reset,sendCharacter,sendPassphrase,sendPin,sendWord,wipe) currently do nothing. If the underlying GridPlus device truly doesn’t support these operations, it’s better for them to throw descriptive errors so callers don’t assume the action succeeded.Consider updating each to immediately throw an error like
"GridPlus does not support <methodName> operation"to make the behavior explicit.
🧹 Nitpick comments (3)
packages/hdwallet-gridplus/src/adapter.ts (2)
7-9:name/passwordchoice is part of the cryptographic client identity — document this coupling.
privKeyis derived fromsha256(deviceId + password + name), and is then passed intonew Client({ name, baseUrl, privKey, deviceId }). This means:
- Changing
namein the future (e.g. rebranding) or altering thepasswordconvention will changeprivKeyand effectively orphan existing pairings.- Callers must consistently pass the same
passwordon reconnect, or the derived identity will not match the one used during pairing.Consider adding a short comment near the
name/privKeydefinition to make it explicit that these values are part of the stable client identity and are not cosmetic, so future refactors don’t inadvertently break existing GridPlus pairings.Also applies to: 23-27
23-49: VerifyClientreuse behavior and keyring integration for multi-session/multi-device flows.Two related concerns in
connectDevice/pairDevice:
Client reuse:
- On the first
connectDevicecall, you buildprivKeyfrom the provideddeviceId/passwordand create aClient.- On subsequent
connectDevicecalls you always reuse the existingClientand only callresetActiveWallets(), even if a differentdeviceIdorpasswordis passed.- If this adapter is ever used for more than one Lattice, or if the user changes the password, you may end up reusing a
Clientwhose internal identity no longer matches the requested device/password.If multi-device or password-change flows are in scope, consider re-instantiating the client when the
deviceId/passwordtuple changes instead of always reusing it.Keyring registration:
pairDeviceadds the newGridPlusHDWalletto the keyring, butconnectDevicedoes not.- For already-paired devices, make sure the surrounding code either:
- Reuses the previously-registered wallet instance from the keyring, or
- Explicitly calls
keyring.addwhenisPairedis true so paired devices discovered viaconnectDeviceare available through the keyring.Please double-check the intended adapter usage patterns (especially for reconnects and multiple devices) and adjust if needed.
packages/hdwallet-gridplus/src/gridplus.ts (1)
374-420:getPublicKeysflow looks correct; confirm curve/coin usage and xpub conversion.The implementation is generally solid:
- Uses
curveto select the appropriateConstants.GET_ADDR_FLAGS(SECP256K1_XPUB vs ED25519_PUB).- Calls
client.getAddresseswithn: 1and validates a result is returned.- Normalizes the first entry to a string (xpub or hex pubkey).
- For secp256k1 +
coin, adjusts the xpub version viacore.scriptTypeToAccountType[scriptType]andcore.convertXpubVersion.- Swallows errors per entry and returns
nullfor failures, which matches a “best-effort” multi-key API.A couple of points to verify:
- That all current call sites only use this for curves/coins the GridPlus SDK supports with these flags (especially non-UTXO, non-Bitcoin coins).
- That
core.convertXpubVersioncorrectly handlesaccountTypebeingundefinedwhenscriptTypeisn’t supplied.If those assumptions hold, the method behavior looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (31)
examples/sandbox/package.json(2 hunks)integration/package.json(1 hunks)lerna.json(1 hunks)packages/hdwallet-coinbase/package.json(2 hunks)packages/hdwallet-core/package.json(2 hunks)packages/hdwallet-gridplus/package.json(2 hunks)packages/hdwallet-gridplus/src/adapter.ts(2 hunks)packages/hdwallet-gridplus/src/bitcoin.ts(1 hunks)packages/hdwallet-gridplus/src/gridplus.ts(6 hunks)packages/hdwallet-gridplus/src/transport.ts(0 hunks)packages/hdwallet-keepkey-chromeusb/package.json(2 hunks)packages/hdwallet-keepkey-electron/package.json(2 hunks)packages/hdwallet-keepkey-nodehid/package.json(2 hunks)packages/hdwallet-keepkey-nodewebusb/package.json(2 hunks)packages/hdwallet-keepkey-tcp/package.json(2 hunks)packages/hdwallet-keepkey-webusb/package.json(2 hunks)packages/hdwallet-keepkey/package.json(3 hunks)packages/hdwallet-keplr/package.json(2 hunks)packages/hdwallet-ledger-webhid/package.json(2 hunks)packages/hdwallet-ledger-webusb/package.json(3 hunks)packages/hdwallet-ledger/package.json(3 hunks)packages/hdwallet-metamask-multichain/package.json(2 hunks)packages/hdwallet-native-vault/package.json(2 hunks)packages/hdwallet-native/package.json(2 hunks)packages/hdwallet-phantom/package.json(2 hunks)packages/hdwallet-portis/package.json(2 hunks)packages/hdwallet-trezor-connect/package.json(2 hunks)packages/hdwallet-trezor/package.json(2 hunks)packages/hdwallet-vultisig/package.json(2 hunks)packages/hdwallet-walletconnect/package.json(2 hunks)packages/hdwallet-walletconnectV2/package.json(2 hunks)
💤 Files with no reviewable changes (1)
- packages/hdwallet-gridplus/src/transport.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/hdwallet-keepkey-nodehid/package.json
- packages/hdwallet-native/package.json
- packages/hdwallet-phantom/package.json
- packages/hdwallet-trezor-connect/package.json
- packages/hdwallet-ledger-webusb/package.json
- packages/hdwallet-gridplus/package.json
- packages/hdwallet-native-vault/package.json
- packages/hdwallet-ledger/package.json
- packages/hdwallet-keplr/package.json
- packages/hdwallet-walletconnect/package.json
- packages/hdwallet-vultisig/package.json
- packages/hdwallet-portis/package.json
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
📚 Learning: 2025-08-07T15:24:34.076Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: examples/sandbox/package.json:15-31
Timestamp: 2025-08-07T15:24:34.076Z
Learning: Lerna v6+ supports workspace protocol syntax (workspace:*) but does not automatically convert exact versions to workspace protocol during version bumps. It only preserves existing workspace protocol syntax. Teams using Lerna for automated version bumps would need manual conversion to use workspace protocol, which negates automation benefits.
Applied to files:
lerna.json
📚 Learning: 2025-08-07T15:24:19.530Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/package.json:3-3
Timestamp: 2025-08-07T15:24:19.530Z
Learning: In the shapeshiftoss/hdwallet monorepo, the team runs `yarn build` before bumping versions and also before publishing packages. This ensures that dist/ artifacts (including UMD bundles) are properly regenerated after dependency updates.
Applied to files:
packages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-core/package.jsonintegration/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-tcp/package.jsonpackages/hdwallet-gridplus/src/bitcoin.tspackages/hdwallet-coinbase/package.jsonpackages/hdwallet-trezor/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.json
📚 Learning: 2025-08-07T15:23:54.858Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-coinbase/package.json:18-18
Timestamp: 2025-08-07T15:23:54.858Z
Learning: In the shapeshiftoss/hdwallet monorepo, package version bumps are done in PRs before publishing. The packages are published after the PR is merged, so dependency versions may reference unpublished versions during the PR review phase. This is expected behavior in their release workflow.
Applied to files:
packages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-core/package.jsonintegration/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-tcp/package.jsonpackages/hdwallet-coinbase/package.jsonpackages/hdwallet-trezor/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.json
📚 Learning: 2025-08-07T15:27:03.179Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/package.json:36-36
Timestamp: 2025-08-07T15:27:03.179Z
Learning: In the shapeshiftoss/hdwallet monorepo, the ledgerhq/hw-transport dependency in packages/hdwallet-ledger/package.json is pinned to an exact version (without caret) due to type mismatches that occur with newer versions. Other Ledger dependencies can safely use caret ranges.
Applied to files:
packages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-core/package.jsonintegration/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-tcp/package.jsonpackages/hdwallet-coinbase/package.jsonpackages/hdwallet-trezor/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.jsonpackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-08-07T15:47:29.207Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/src/transport.ts:10-10
Timestamp: 2025-08-07T15:47:29.207Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the code works locally without TypeScript errors but has issues in CI environment. Using ts-expect-error would fail locally since there are no actual errors to suppress.
Applied to files:
packages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-core/package.jsonintegration/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-tcp/package.jsonpackages/hdwallet-gridplus/src/bitcoin.tspackages/hdwallet-coinbase/package.jsonpackages/hdwallet-trezor/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.json
📚 Learning: 2025-08-07T15:47:26.835Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/src/transport.ts:12-12
Timestamp: 2025-08-07T15:47:26.835Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the CI environment has different type checking behavior than local development. The code works locally without errors, but CI reports type issues, so ts-ignore is necessary to suppress the inconsistent type checking across environments.
Applied to files:
packages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-metamask-multichain/package.jsonintegration/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-tcp/package.jsonpackages/hdwallet-gridplus/src/bitcoin.tspackages/hdwallet-coinbase/package.jsonpackages/hdwallet-trezor/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.json
📚 Learning: 2025-10-15T23:22:26.842Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Applied to files:
packages/hdwallet-gridplus/src/adapter.tspackages/hdwallet-gridplus/src/bitcoin.tspackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-10-14T20:59:48.768Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
Applied to files:
packages/hdwallet-gridplus/src/adapter.tspackages/hdwallet-gridplus/src/bitcoin.tspackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-11-20T11:04:44.808Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 737
File: packages/hdwallet-trezor/src/ethereum.ts:122-138
Timestamp: 2025-11-20T11:04:44.808Z
Learning: In packages/hdwallet-trezor/src/ethereum.ts, the ethSignTypedData function correctly returns the signature from res.payload.signature without adding a "0x" prefix. This works correctly in practice and has been tested, despite appearing inconsistent with ethSignMessage which does add the prefix. The Trezor Connect ethereumSignTypedData response already provides the signature in the correct format for consumption.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
🧬 Code graph analysis (2)
packages/hdwallet-gridplus/src/bitcoin.ts (4)
packages/hdwallet-gridplus/src/utils.ts (1)
getCompressedPubkey(6-17)packages/hdwallet-gridplus/src/gridplus.ts (2)
btcGetAddress(422-425)btcSignTx(427-430)packages/hdwallet-native/src/bitcoin.ts (2)
btcGetAddress(177-187)btcSignTx(189-295)packages/hdwallet-core/src/bitcoin.ts (3)
BTCGetAddress(20-25)BTCSignTx(222-230)BTCSignedTx(237-242)
packages/hdwallet-gridplus/src/gridplus.ts (5)
packages/hdwallet-core/src/ethereum.ts (9)
ETHWallet(197-216)ETHGetAddress(43-46)Address(15-15)ETHSignTx(48-82)ETHSignedTx(88-97)ETHSignTypedData(109-112)ETHSignedTypedData(114-117)ETHSignMessage(99-102)ETHSignedMessage(104-107)packages/hdwallet-core/src/mayachain.ts (4)
MayachainWallet(94-99)MayachainGetAddress(4-7)MayachainSignTx(56-63)MayachainSignedTx(65-70)packages/hdwallet-core/src/solana.ts (3)
SolanaGetAddress(14-18)SolanaSignTx(37-53)SolanaSignedTx(55-58)packages/hdwallet-core/src/wallet.ts (2)
GetPublicKey(22-28)PublicKey(30-32)packages/hdwallet-core/src/cosmos.ts (3)
CosmosGetAddress(4-7)CosmosSignTx(56-63)CosmosSignedTx(65-70)
🪛 ESLint
packages/hdwallet-gridplus/src/bitcoin.ts
[error] 57-57: Insert ;
(prettier/prettier)
packages/hdwallet-gridplus/src/gridplus.ts
[error] 12-12: Insert ;
(prettier/prettier)
[error] 370-370: Insert ;
(prettier/prettier)
[error] 371-371: Insert ;
(prettier/prettier)
⏰ 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 and Release
🔇 Additional comments (18)
lerna.json (1)
3-3: Version bump consistent with PR scope.The pre-release version
1.62.10-alpha.1is appropriate for the gridplus cleanup work and aligns with version bumps across the monorepo packages as described in the PR objectives.packages/hdwallet-walletconnectV2/package.json (1)
3-3: Version and build script updates are consistent and well-coordinated.The changes align with the PR objectives and established patterns: clean now removes
node_modules, andprepublishOnlyexplicitly clears dist before build, eliminating the need for a separate yarn clean step. Dependency versions are properly synchronized.Also applies to: 13-15, 18-18
packages/hdwallet-keepkey-tcp/package.json (1)
3-3: Coordinated version and dependency updates.All changes follow the established pattern for this release cycle. Dependency versions are correctly synchronized with the package version bump.
Also applies to: 13-14, 17-18
packages/hdwallet-keepkey-chromeusb/package.json (1)
3-3: Consistent pattern maintained.Also applies to: 13-14, 17-18
packages/hdwallet-core/package.json (1)
3-3: Core package version and build script updates are correct.The core package properly updates scripts without modifying external dependencies. Downstream packages depending on this version (1.62.10-alpha.1) have been correctly synchronized.
Also applies to: 13-14
packages/hdwallet-keepkey/package.json (1)
3-3: All changes consistent with coordinated monorepo update.Also applies to: 14-15, 24-24
packages/hdwallet-keepkey-electron/package.json (1)
3-3: Electron integration package properly updated with coordinated dependencies.Also applies to: 13-14, 17-17
packages/hdwallet-coinbase/package.json (1)
3-3: Wallet integration package consistent with monorepo updates.Also applies to: 13-14, 18-18
packages/hdwallet-keepkey-webusb/package.json (1)
3-3: WebUSB transport variant properly synchronized with other packages.Also applies to: 13-14, 17-18
packages/hdwallet-keepkey-nodewebusb/package.json (1)
3-3: Version and script changes align with PR standardization.The version bump to 1.62.10-alpha.1, script updates, and dependency versions are consistent across the monorepo. The shift from cleaning
tsconfig.tsbuildinfotonode_modulesaligns with the PR's build-info standardization.Also applies to: 13-14, 17-18
packages/hdwallet-metamask-multichain/package.json (1)
3-3: Consistent with PR pattern.All updates follow the established version and script standardization across the monorepo.
Also applies to: 13-14, 20-20
packages/hdwallet-trezor/package.json (1)
3-3: Consistent with PR standardization.Version and script changes align with other packages in the monorepo update.
Also applies to: 14-15, 20-20
integration/package.json (1)
3-3: Version and dependency updates consistent; clean script appropriately adds .cache removal.Integration package's expanded clean script to include
.cacheis reasonable for build artifacts specific to integration tests. All version updates aligned to 1.62.10-alpha.1.Also applies to: 8-8, 14-23
packages/hdwallet-ledger-webhid/package.json (1)
3-3: Consistent updates; external Ledger dependencies properly maintained.Version bump and script standardization align with PR pattern. Third-party Ledger dependencies remain at appropriate pinned versions.
Also applies to: 13-14, 21-22
examples/sandbox/package.json (1)
3-3: Comprehensive dependency updates; clean script appropriately includes build artifacts.All 18 hdwallet dependencies properly updated to 1.62.10-alpha.1. Clean script correctly removes the Vite output directory (
public/) along with build artifacts, aligning with the sandbox build flow.Also applies to: 10-10, 15-32
packages/hdwallet-gridplus/src/gridplus.ts (3)
272-306: Client-backed GridPlusHDWallet wiring looks good.Storing the
Clienton the wallet and shifting to a client-centric construction pattern (new GridPlusHDWallet(client)) aligns with the new adapter flow and simplifies downstream chain-specific calls. The implements list for BTC/Cosmos/ETH/Mayachain/Solana/Thorchain also looks consistent with the core interfaces.
320-363: Metadata and connection helpers are straightforward and consistent.
getDeviceID,getFeatures, andgetFirmwareVersioncorrectly delegate to theClientwith appropriate “device not connected” guards.getModel/getLabelreturning static strings is fine given a single supported model.isInitializedandisLockedare simple; as long as “initialized” is intentionally defined as “we have a client instance”, this is acceptable.ping/disconnectbehavior is minimal but clear.No functional issues spotted here.
422-514: Chain-specific wrappers are clean delegations with proper client guards.The BTC/ETH/Solana/Cosmos/Thorchain/Mayachain methods:
- Consistently guard against a missing
clientwith"Device not connected"errors.- Delegate to the respective per-chain modules (
btc,eth,solana,cosmos,thorchain,mayachain) without additional logic, which keeps this class focused on device/client orchestration.- Explicitly throw for unimplemented BTC/ETH message verification/signing operations, which is clearer than silent failures.
This block looks good as-is.
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.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/hdwallet-gridplus/src/bitcoin.ts (2)
179-180: Update the misleading comment —singleHash: trueis the correct GridPlus integration.The comment suggests
singleHash: trueis a temporary workaround pending a GridPlus fix, but as clarified in the previous review, this is actually the correct and required integration pattern. GridPlus's signing API expects a single SHA-256 input and returns raw 64-byte r||s signatures compatible with bitcoinjs-lib's SignerAsync interface.Update the comment to clarify the integration pattern:
- // single sha256 hash and allow device to perform second sha256 hash when signing (pending gridplus fix for hashType.NONE) + // GridPlus requires single SHA-256 hash input (singleHash: true) and returns raw 64-byte r||s signatures await psbt.signInputAsync(i, signer, undefined, true);
183-194: Add THORChain vout ordering validation.The GridPlus implementation is missing THORChain vout ordering validation that's present in all other wallet implementations (native, phantom, vultisig, keepkey). When
msg.vaultAddressis set, you should validate output ordering usingcore.validateVoutOrdering(msg, tx)before returning.Add validation before the return statement:
const tx = psbt.extractTransaction(true); - console.log({ msg, tx, psbt }); - const signatures = psbt.data.inputs.map((input) => input.partialSig ? Buffer.from(input.partialSig[0].signature).toString("hex") : "" ); + if (msg.vaultAddress && !core.validateVoutOrdering(msg, tx)) { + throw new Error("Transaction failed THORChain vout ordering validation"); + } + return { signatures, serializedTx: tx.toHex() };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
packages/hdwallet-gridplus/src/bitcoin.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
📚 Learning: 2025-10-15T23:22:26.842Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-11-20T11:04:44.808Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 737
File: packages/hdwallet-trezor/src/ethereum.ts:122-138
Timestamp: 2025-11-20T11:04:44.808Z
Learning: In packages/hdwallet-trezor/src/ethereum.ts, the ethSignTypedData function correctly returns the signature from res.payload.signature without adding a "0x" prefix. This works correctly in practice and has been tested, despite appearing inconsistent with ethSignMessage which does add the prefix. The Trezor Connect ethereumSignTypedData response already provides the signature in the correct format for consumption.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-10-14T20:59:48.768Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:47:29.207Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/src/transport.ts:10-10
Timestamp: 2025-08-07T15:47:29.207Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the code works locally without TypeScript errors but has issues in CI environment. Using ts-expect-error would fail locally since there are no actual errors to suppress.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:47:26.835Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/src/transport.ts:12-12
Timestamp: 2025-08-07T15:47:26.835Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the CI environment has different type checking behavior than local development. The code works locally without errors, but CI reports type issues, so ts-ignore is necessary to suppress the inconsistent type checking across environments.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:24:19.530Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/package.json:3-3
Timestamp: 2025-08-07T15:24:19.530Z
Learning: In the shapeshiftoss/hdwallet monorepo, the team runs `yarn build` before bumping versions and also before publishing packages. This ensures that dist/ artifacts (including UMD bundles) are properly regenerated after dependency updates.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
🪛 ESLint
packages/hdwallet-gridplus/src/bitcoin.ts
[error] 187-187: Unexpected console statement.
(no-console)
⏰ 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 and Release
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.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/hdwallet-keepkey-webusb/package.json (1)
13-14: Verify script changes are intentional across the monorepo.Same script workflow change as
hdwallet-keepkey:cleannow removesnode_modules(nottsbuildinfo), andprepublishOnlyskips theyarn cleancall. Ensure this aligns with the team's build and publish strategy.packages/hdwallet-keepkey-electron/package.json (1)
13-14: Verify script changes are intentional across the monorepo.Same script workflow change:
cleanremovesnode_modulesinstead oftsbuildinfo, andprepublishOnlyno longer callsyarn clean. Consistent with the other keepkey packages but warrants verification of the design intent.examples/sandbox/index.ts (1)
241-262: Critical: Remove "#" prefix from getElementById calls.
document.getElementById()expects a plain ID string, not a CSS selector. The "#" prefix will cause these calls to returnnull, leading to runtime errors when attempting to setclassName.Apply this diff to fix the issue:
- document.getElementById("#deviceIdModal").className = "modal opened"; + document.getElementById("deviceIdModal").className = "modal opened"; window["deviceIdEntered"] = async function () { - const input = document.getElementById("#deviceIdInput") as HTMLInputElement; + const input = document.getElementById("deviceIdInput") as HTMLInputElement; deviceId = input.value; - document.getElementById("#deviceIdModal").className = "modal"; + document.getElementById("deviceIdModal").className = "modal"; wallet = await gridplusAdapter.connectDevice(deviceId); if (wallet) { window["wallet"] = wallet; $("#keyring select").val(await wallet.getDeviceID()); } else { - document.getElementById("#pairingCodeModal").className = "modal opened"; + document.getElementById("pairingCodeModal").className = "modal opened"; } }; window["pairingCodeEntered"] = async function () { - const input = document.getElementById("#pairingCodeInput") as HTMLInputElement; + const input = document.getElementById("pairingCodeInput") as HTMLInputElement; - document.getElementById("#pairingCodeModal").className = "modal"; + document.getElementById("pairingCodeModal").className = "modal"; wallet = await gridplusAdapter.pairDevice(input.value); window["wallet"] = wallet; $("#keyring select").val(await wallet.getDeviceID()); };packages/hdwallet-gridplus/src/gridplus.ts (3)
12-12: Add missing semicolon for ZERO_BUFFER (lint fix).ESLint/Prettier requires a trailing semicolon here.
Apply this diff:
-const ZERO_BUFFER = Buffer.alloc(32) +const ZERO_BUFFER = Buffer.alloc(32);
308-318: Replace empty lifecycle stubs with explicit unsupported errors.These lifecycle methods silently do nothing, which could lead to confusion when attempting operations like device initialization, recovery, or wipe. If GridPlus does not support these operations, throw descriptive errors to make this explicit.
Apply this diff:
- async cancel(): Promise<void> {} - async clearSession(): Promise<void> {} - async initialize(): Promise<void> {} - async loadDevice(): Promise<void> {} - async recover(): Promise<void> {} - async reset(): Promise<void> {} - async sendCharacter(): Promise<void> {} - async sendPassphrase(): Promise<void> {} - async sendPin(): Promise<void> {} - async sendWord(): Promise<void> {} - async wipe(): Promise<void> {} + async cancel(): Promise<void> { + throw new Error("GridPlus does not support cancel operation"); + } + async clearSession(): Promise<void> { + throw new Error("GridPlus does not support clearSession operation"); + } + async initialize(): Promise<void> { + throw new Error("GridPlus does not support initialize operation"); + } + async loadDevice(): Promise<void> { + throw new Error("GridPlus does not support loadDevice operation"); + } + async recover(): Promise<void> { + throw new Error("GridPlus does not support recover operation"); + } + async reset(): Promise<void> { + throw new Error("GridPlus does not support reset operation"); + } + async sendCharacter(): Promise<void> { + throw new Error("GridPlus does not support sendCharacter operation"); + } + async sendPassphrase(): Promise<void> { + throw new Error("GridPlus does not support sendPassphrase operation"); + } + async sendPin(): Promise<void> { + throw new Error("GridPlus does not support sendPin operation"); + } + async sendWord(): Promise<void> { + throw new Error("GridPlus does not support sendWord operation"); + } + async wipe(): Promise<void> { + throw new Error("GridPlus does not support wipe operation"); + }
370-371: Fix missing semicolons in getActiveWallet (lint).ESLint/Prettier requires trailing semicolons on return statements.
Apply this diff:
- if (!external.uid.equals(ZERO_BUFFER)) return external.uid.toString("hex") - if (!internal.uid.equals(ZERO_BUFFER)) return internal.uid.toString("hex") + if (!external.uid.equals(ZERO_BUFFER)) return external.uid.toString("hex"); + if (!internal.uid.equals(ZERO_BUFFER)) return internal.uid.toString("hex");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (29)
examples/sandbox/index.ts(4 hunks)examples/sandbox/package.json(2 hunks)integration/package.json(1 hunks)lerna.json(1 hunks)packages/hdwallet-coinbase/package.json(2 hunks)packages/hdwallet-core/package.json(2 hunks)packages/hdwallet-gridplus/package.json(2 hunks)packages/hdwallet-gridplus/src/gridplus.ts(6 hunks)packages/hdwallet-keepkey-chromeusb/package.json(2 hunks)packages/hdwallet-keepkey-electron/package.json(2 hunks)packages/hdwallet-keepkey-nodehid/package.json(2 hunks)packages/hdwallet-keepkey-nodewebusb/package.json(2 hunks)packages/hdwallet-keepkey-tcp/package.json(2 hunks)packages/hdwallet-keepkey-webusb/package.json(2 hunks)packages/hdwallet-keepkey/package.json(3 hunks)packages/hdwallet-keplr/package.json(2 hunks)packages/hdwallet-ledger-webhid/package.json(2 hunks)packages/hdwallet-ledger-webusb/package.json(3 hunks)packages/hdwallet-ledger/package.json(3 hunks)packages/hdwallet-metamask-multichain/package.json(2 hunks)packages/hdwallet-native-vault/package.json(2 hunks)packages/hdwallet-native/package.json(2 hunks)packages/hdwallet-phantom/package.json(2 hunks)packages/hdwallet-portis/package.json(2 hunks)packages/hdwallet-trezor-connect/package.json(2 hunks)packages/hdwallet-trezor/package.json(2 hunks)packages/hdwallet-vultisig/package.json(2 hunks)packages/hdwallet-walletconnect/package.json(2 hunks)packages/hdwallet-walletconnectV2/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/hdwallet-core/package.json
- packages/hdwallet-native-vault/package.json
- packages/hdwallet-keepkey-nodewebusb/package.json
- packages/hdwallet-walletconnectV2/package.json
- packages/hdwallet-keplr/package.json
- packages/hdwallet-native/package.json
- packages/hdwallet-trezor/package.json
- lerna.json
- packages/hdwallet-keepkey-tcp/package.json
- packages/hdwallet-keepkey-nodehid/package.json
- integration/package.json
- packages/hdwallet-phantom/package.json
- packages/hdwallet-portis/package.json
- packages/hdwallet-metamask-multichain/package.json
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
📚 Learning: 2025-08-07T15:27:03.179Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/package.json:36-36
Timestamp: 2025-08-07T15:27:03.179Z
Learning: In the shapeshiftoss/hdwallet monorepo, the ledgerhq/hw-transport dependency in packages/hdwallet-ledger/package.json is pinned to an exact version (without caret) due to type mismatches that occur with newer versions. Other Ledger dependencies can safely use caret ranges.
Applied to files:
packages/hdwallet-ledger-webhid/package.jsonexamples/sandbox/index.tspackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-ledger-webusb/package.jsonpackages/hdwallet-walletconnect/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonpackages/hdwallet-vultisig/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-gridplus/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-coinbase/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-08-07T15:23:54.858Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-coinbase/package.json:18-18
Timestamp: 2025-08-07T15:23:54.858Z
Learning: In the shapeshiftoss/hdwallet monorepo, package version bumps are done in PRs before publishing. The packages are published after the PR is merged, so dependency versions may reference unpublished versions during the PR review phase. This is expected behavior in their release workflow.
Applied to files:
packages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-ledger-webusb/package.jsonpackages/hdwallet-walletconnect/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonpackages/hdwallet-vultisig/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-gridplus/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-coinbase/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey-webusb/package.json
📚 Learning: 2025-08-07T15:24:19.530Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/package.json:3-3
Timestamp: 2025-08-07T15:24:19.530Z
Learning: In the shapeshiftoss/hdwallet monorepo, the team runs `yarn build` before bumping versions and also before publishing packages. This ensures that dist/ artifacts (including UMD bundles) are properly regenerated after dependency updates.
Applied to files:
packages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-ledger-webusb/package.jsonpackages/hdwallet-walletconnect/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonpackages/hdwallet-vultisig/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-gridplus/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-coinbase/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey-webusb/package.json
📚 Learning: 2025-08-07T15:47:29.207Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/src/transport.ts:10-10
Timestamp: 2025-08-07T15:47:29.207Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the code works locally without TypeScript errors but has issues in CI environment. Using ts-expect-error would fail locally since there are no actual errors to suppress.
Applied to files:
packages/hdwallet-ledger-webhid/package.jsonexamples/sandbox/index.tspackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-ledger-webusb/package.jsonpackages/hdwallet-walletconnect/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonpackages/hdwallet-vultisig/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-gridplus/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-coinbase/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey-webusb/package.jsonpackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-08-07T15:47:26.835Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/src/transport.ts:12-12
Timestamp: 2025-08-07T15:47:26.835Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the CI environment has different type checking behavior than local development. The code works locally without errors, but CI reports type issues, so ts-ignore is necessary to suppress the inconsistent type checking across environments.
Applied to files:
packages/hdwallet-ledger-webhid/package.jsonexamples/sandbox/index.tspackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-ledger-webusb/package.jsonpackages/hdwallet-walletconnect/package.jsonpackages/hdwallet-keepkey/package.jsonpackages/hdwallet-keepkey-chromeusb/package.jsonpackages/hdwallet-vultisig/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-coinbase/package.jsonexamples/sandbox/package.jsonpackages/hdwallet-keepkey-webusb/package.json
📚 Learning: 2025-10-15T23:22:26.842Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Applied to files:
examples/sandbox/index.tspackages/hdwallet-gridplus/package.jsonpackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-10-14T20:59:48.768Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
Applied to files:
examples/sandbox/index.tspackages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-11-20T11:04:44.808Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 737
File: packages/hdwallet-trezor/src/ethereum.ts:122-138
Timestamp: 2025-11-20T11:04:44.808Z
Learning: In packages/hdwallet-trezor/src/ethereum.ts, the ethSignTypedData function correctly returns the signature from res.payload.signature without adding a "0x" prefix. This works correctly in practice and has been tested, despite appearing inconsistent with ethSignMessage which does add the prefix. The Trezor Connect ethereumSignTypedData response already provides the signature in the correct format for consumption.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
🧬 Code graph analysis (1)
examples/sandbox/index.ts (1)
packages/hdwallet-native/src/crypto/EncryptedWallet.ts (1)
deviceId(67-69)
⏰ 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 and Release
🔇 Additional comments (24)
packages/hdwallet-walletconnect/package.json (1)
3-3: LGTM! Version bump and script harmonization are consistent with PR objectives.The changes align well with the broader PR effort:
- Version bump (line 3): Updated from 1.62.9 to 1.62.10-alpha.2, consistent with the monorepo versioning strategy.
- Dependency consistency (line 18):
@shapeshiftoss/hdwallet-corematches this package's version (1.62.10-alpha.2).- Script updates (lines 13, 15): Clean script now removes
node_modules(fulfilling the PR objective: "Add back removal ofnode_modulesonyarn cleanscripts"), andprepublishOnlyexplicitly clears dist before building—both standard practices for build hygiene and script harmonization across the monorepo.Also applies to: 13-13, 15-15, 18-18
packages/hdwallet-coinbase/package.json (2)
3-3: Version bump aligns with coordinated monorepo release.The version and dependency updates to 1.62.10-alpha.2 are consistent with the PR's coordinated package updates. This follows the established workflow where package versions reference unpublished versions during the PR phase.
Also applies to: 18-18
13-14: Clarify the intentional removal oftsconfig.tsbuildinfocleanup.The
cleanscript now removesnode_modulesinstead oftsconfig.tsbuildinfo, and theprepublishOnlyscript no longer callsyarn clean(thus avoiding node_modules reinstall during publish). While this optimization makes sense—keeping node_modules intact during prepublish is efficient—it also meanstsconfig.tsbuildinfois no longer explicitly cleaned.Since tsbuildinfo is a build artifact that TypeScript will regenerate on the next build, this change is likely safe. However, please confirm this is intentional (especially if the tsbuildinfo cleanup was previously needed for CI correctness).
packages/hdwallet-keepkey-chromeusb/package.json (2)
3-3: Version bump and dependency updates aligned with monorepo release workflow.The version and dependency changes align with the expected behavior in this monorepo's release workflow, where packages are version-bumped in PRs and published after merge. Based on learnings, this is standard practice.
Also applies to: 17-18
13-14: Clean and prepublishOnly scripts strategically separated to preserve node_modules during publish.The script refactoring is well-designed:
- Line 13: The
cleanscript now removesnode_modulesper PR objectives ("Add back removal ofnode_modulesonyarn cleanscripts").- Line 14: The
prepublishOnlyscript inlinesrm -rf distinstead of callingyarn clean, preventing accidental removal ofnode_modulesduring the publish flow. This is a correct separation of concerns.Note:
tsconfig.tsbuildinfois no longer removed byclean. Verify this is intentional and doesn't leave stale artifacts that could affect incremental builds across multiple runs.Please confirm:
- The removal of
tsconfig.tsbuildinfofrom thecleanscript is intentional and doesn't impact incremental build behavior.- This pattern (removing
node_modulesfromcleanand updatingprepublishOnly) is consistent across other packages touched in this PR, as mentioned in the AI summary.packages/hdwallet-trezor-connect/package.json (1)
3-3: Changes are consistent and properly aligned across the monorepo.Verification confirms that all related packages follow the same patterns:
- Version 1.62.10-alpha.2 is applied consistently
- Clean scripts uniformly include
node_modulesremoval across all 24 packages- prepublishOnly scripts follow the standardized
rm -rf dist && yarn buildpattern throughoutThe changes in
packages/hdwallet-trezor-connect/package.jsonare correct and properly synchronized with monorepo conventions.packages/hdwallet-gridplus/package.json (3)
3-3: Version and dependency bumps align with monorepo release workflow.The alpha version bump and updated
@shapeshiftoss/hdwallet-coredependency match the expected pre-release workflow for this monorepo.Also applies to: 23-23
14-15: Script changes address prior publish breakage.The refactored
cleanandprepublishOnlyscripts resolve the earlier issue:cleannow aggressively removesnode_modules, whileprepublishOnlyindependently runsrm -rf dist && yarn buildwithout callingclean. This decouples the full clean from the publish flow. Based on learnings from PR #726, this aligns with the team's approach.Please confirm the script changes work as intended and that publish workflows complete successfully. Also verify that the removal of
tsconfig.tsbuildinfofrom the oldcleanscript was intentional (it no longer appears in the new version).
20-20: Now I need to verify whether RLP is actually used in the ethereum.ts file:Now let me check if RLP is actually being used in the ethereum.ts file by looking at the code:Based on the evidence I've gathered, I can now determine that the @ethereumjs/rlp dependency has been confirmed to be imported in the ethereum.ts file. The import statement was found at line 2. However, I need to verify if it's actually being used in the code rather than being a dead import. Let me generate the final response based on what I know.Confirm @ethereumjs/rlp dependency is imported in packages/hdwallet-gridplus/src/ethereum.ts.
The import statement
import { RLP } from "@ethereumjs/rlp"was confirmed at line 2 ofpackages/hdwallet-gridplus/src/ethereum.ts. This dependency is now properly added to the gridplus package. The library provides standard RLP.encode and RLP.decode functions for serializing Ethereum data.packages/hdwallet-vultisig/package.json (2)
3-3: Version and dependency updates look appropriate for PR phase.The alpha version bump (1.62.9 → 1.62.10-alpha.2) and synchronized @shapeshiftoss/hdwallet-core dependency update are consistent with the broader monorepo refactoring. Per prior learnings, pre-release version references during PR review are expected before merge.
Also applies to: 20-20
13-14: No issues found—script changes align with monorepo standardization.Verification confirms that both script changes in
packages/hdwallet-vultisig/package.jsonare consistent with the established monorepo pattern across all 24+ packages:
- Line 13 (
clean): The pattern"rm -rf dist node_modules"is standard across the entire monorepo. No packages includetsconfig.tsbuildinfoin their clean scripts, confirming this represents intentional standardization of build-info handling.- Line 14 (
prepublishOnly): The pattern"rm -rf dist && yarn build"matches all other packages in the monorepo, including core packages likehdwallet-core,hdwallet-ledger, andhdwallet-keepkey. This aligns with the established publish workflow.The changes are correct and consistent with monorepo conventions.
packages/hdwallet-keepkey/package.json (2)
3-3: Version and dependency bumps align with PR objectives.The version bump to
1.62.10-alpha.2and updated@shapeshiftoss/hdwallet-coredependency are consistent with the PR's GridPlus cleanup scope. Per the monorepo workflow, version bumps in PRs are expected before publishing.Also applies to: 24-24
14-15: tsbuildinfo cleanup is maintained; clarify the node_modules removal pattern.The verification confirms that
tsbuildinfocleanup is not broken by these changes. TheprepublishOnlyscript removesdistbefore building, and sincetsbuildInfoFileis configured as./dist/tsconfig.tsbuildinfo, the incremental build cache is still cleaned. The functional behavior is identical to the previous flow.However, the
cleanscript now removesnode_modules, which is unusual. This change is consistent across all keepkey-related packages (chromeusb, electron, nodehid, webusb), suggesting it's intentional. But sinceprepublishOnlyonly removesdist(not callingyarn clean), this doesn't impact the publish workflow.Clarification needed: Why does
cleanremovenode_modules? This is atypical and should be documented. Confirm this aligns with the monorepo's dependency management strategy.packages/hdwallet-keepkey-webusb/package.json (1)
3-3: Version and dependency updates are consistent.Version bumped to
1.62.10-alpha.2with corresponding updates to both@shapeshiftoss/hdwallet-coreand@shapeshiftoss/hdwallet-keepkey. Follows expected monorepo release workflow.Also applies to: 17-18
packages/hdwallet-keepkey-electron/package.json (1)
3-3: Version and dependency updates are consistent.Version bumped to
1.62.10-alpha.2with@shapeshiftoss/hdwallet-keepkeyupdated accordingly. Aligns with the monorepo's coordinated version bump across the GridPlus cleanup PR.Also applies to: 17-17
packages/hdwallet-ledger-webhid/package.json (1)
3-3: Version bump and script standardization look good.The version bump to 1.62.10-alpha.2 and the updated build scripts are consistent with the coordinated release across hdwallet packages.
Based on learnings, version bumps in PRs before publishing are part of the expected release workflow.
Also applies to: 13-14, 21-22
packages/hdwallet-ledger/package.json (1)
3-3: LGTM - Consistent with monorepo-wide version bump.The version bump to 1.62.10-alpha.2 and script updates align with the coordinated release across all hdwallet packages.
Also applies to: 14-15, 28-28
packages/hdwallet-ledger-webusb/package.json (1)
3-3: LGTM - Version bump aligns with monorepo release.Version and script changes are consistent with the coordinated 1.62.10-alpha.2 release.
Also applies to: 13-14, 23-24
examples/sandbox/package.json (1)
3-3: LGTM - Sandbox dependencies updated for coordinated release.All hdwallet package dependencies are correctly bumped to 1.62.10-alpha.2, aligning with the monorepo-wide release.
Also applies to: 15-32
examples/sandbox/index.ts (1)
246-246: The review comment is based on a misunderstanding of the connectDevice return type.The
connectDevicemethod inpackages/hdwallet-gridplus/src/adapter.ts(line 23) has the return typePromise<GridPlusHDWallet | undefined>. It returns the wallet instance directly, not wrapped in an object like{ wallet, sessionId }. The current code at line 246 that assigns the result directly towalletis correct and matches the actual method signature.Likely an incorrect or invalid review comment.
packages/hdwallet-gridplus/src/gridplus.ts (4)
303-306: LGTM: Client-based architecture.The transition from transport-based to client-based architecture is cleanly implemented. All downstream methods correctly check for client presence before use.
422-514: LGTM: Consistent wallet method implementations.All wallet methods follow a consistent pattern:
- Check client presence before operations
- Delegate to specialized coin modules
- Use type assertions for firmware version checks (e.g.,
assertSolanaFwSupport)- Throw explicit errors for unimplemented methods
The implementation is clean and maintainable.
383-411: Core utilities for xpub conversion are properly implemented.The utilities
scriptTypeToAccountTypeandconvertXpubVersionare correctly exported frompackages/hdwallet-core/src/utxoUtils.tsand already integrated in production code (Ledger wallet). The implementation handles all required coin types (Bitcoin, Litecoin, Dogecoin) with proper error handling and edge case safeguards.
361-363: No action required — GridPlus SDK does not require explicit disconnect.The gridplus-sdk Client does not expose a dedicated disconnect/cleanup method, and normal usage doesn't require an explicit disconnect/close. Setting
this.clienttoundefinedis appropriate for this SDK's design.
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/hdwallet-gridplus/src/gridplus.ts (1)
301-318: Lifecycle stubs andisLocked()could be more explicit about unsupported behavior.These async lifecycle methods currently no-op, and
isLocked()always returnsfalse. That’s acceptable for a minimal integration, but it can hide misuse when callers expect real behavior; throwing explicit “not supported on GridPlus” errors (or at least adding comments) would make limitations clearer.Also applies to: 353-355
🧹 Nitpick comments (2)
packages/hdwallet-gridplus/src/gridplus.ts (2)
12-12: ZERO_BUFFER andgetActiveWalletId()sentinel handling look correct; minor clarity tweak optional.Using a shared ZERO_BUFFER and returning the first non-zero
uidhex fromfetchActiveWallet()is a clean way to surface the active wallet, with an implicitundefinedwhen both are zero; you might optionally add a short comment about the “all-zero uid means no active wallet” assumption or an explicitreturn undefinedfor readability.Also applies to: 365-372
374-420:getPublicKeys: curve-based selection and xpub conversion are reasonable; consider error visibility and future curves.The curve→flag mapping and conditional xpub-version conversion by
coin/scriptTypeare sensible, and failing fast on unsupported curves is fine. The only downside is that any error (including device/SDK issues) is swallowed asnull, which can make debugging harder; consider at least logging or annotating thenullcases, and perhaps adding a brief comment that onlysecp256k1anded25519are intentionally supported here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/hdwallet-gridplus/src/gridplus.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
📚 Learning: 2025-10-15T23:22:26.842Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-08-07T15:27:03.179Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/package.json:36-36
Timestamp: 2025-08-07T15:27:03.179Z
Learning: In the shapeshiftoss/hdwallet monorepo, the ledgerhq/hw-transport dependency in packages/hdwallet-ledger/package.json is pinned to an exact version (without caret) due to type mismatches that occur with newer versions. Other Ledger dependencies can safely use caret ranges.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-10-14T20:59:48.768Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-08-07T15:47:29.207Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/src/transport.ts:10-10
Timestamp: 2025-08-07T15:47:29.207Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the code works locally without TypeScript errors but has issues in CI environment. Using ts-expect-error would fail locally since there are no actual errors to suppress.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-11-20T11:04:44.808Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 737
File: packages/hdwallet-trezor/src/ethereum.ts:122-138
Timestamp: 2025-11-20T11:04:44.808Z
Learning: In packages/hdwallet-trezor/src/ethereum.ts, the ethSignTypedData function correctly returns the signature from res.payload.signature without adding a "0x" prefix. This works correctly in practice and has been tested, despite appearing inconsistent with ethSignMessage which does add the prefix. The Trezor Connect ethereumSignTypedData response already provides the signature in the correct format for consumption.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
📚 Learning: 2025-08-07T15:47:26.835Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/src/transport.ts:12-12
Timestamp: 2025-08-07T15:47:26.835Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the CI environment has different type checking behavior than local development. The code works locally without errors, but CI reports type issues, so ts-ignore is necessary to suppress the inconsistent type checking across environments.
Applied to files:
packages/hdwallet-gridplus/src/gridplus.ts
🧬 Code graph analysis (1)
packages/hdwallet-gridplus/src/gridplus.ts (6)
packages/hdwallet-core/src/ethereum.ts (5)
ETHWallet(197-216)ETHGetAddress(43-46)Address(15-15)ETHSignTx(48-82)ETHSignedTx(88-97)packages/hdwallet-core/src/mayachain.ts (1)
MayachainWallet(94-99)packages/hdwallet-core/src/solana.ts (1)
SolanaWallet(87-94)packages/hdwallet-core/src/thorchain.ts (1)
ThorchainWallet(97-102)packages/hdwallet-core/src/wallet.ts (4)
Ping(34-39)Pong(41-43)GetPublicKey(22-28)PublicKey(30-32)packages/hdwallet-core/src/cosmos.ts (2)
CosmosGetAddress(4-7)CosmosSignTx(56-63)
⏰ 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 and Release
🔇 Additional comments (6)
packages/hdwallet-gridplus/src/gridplus.ts (6)
278-281: Interface expansion for ETH/Mayachain/Solana/Thorchain looks consistent.The added interfaces match the capabilities exposed below (eth*, solana*, thorchain*, mayachain* methods and _supports flags), so the public surface aligns cleanly with the implemented functionality.
320-351: Client-based device metadata and connection state wiring LGTM.
getDeviceID,getFeatures, firmware/model/label,isInitialized,ping, anddisconnectall correctly pivot to the injected client and guard for missing devices where appropriate; no correctness issues stand out.Also applies to: 357-363
422-438: BTC wrappers correctly guard on client and communicate unsupported features.Address/tx signing delegate cleanly to the BTC module with a client presence check, and message sign/verify explicitly throw “not implemented” errors instead of silently failing.
440-462: ETH wrappers cleanly delegate to shared Ethereum helpers.The ETH address/tx/typed-data/message flows are now thin pass-throughs with a consistent “Device not connected” guard, and ETH message verification is clearly marked as unsupported via an error.
476-484: Solana methods guarded by firmware check are well-structured.Requiring a connected client and firmware ≥0.14.0 before delegating Solana address/sign requests is a good guardrail and should fail fast with a clear error on older devices.
486-514: Cosmos/THORChain/Mayachain wrappers follow the same safe delegation pattern.Each method asserts client presence then forwards to the respective chain module, keeping GridPlusHDWallet thin and consistent across supported chains.
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.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/hdwallet-gridplus/src/bitcoin.ts (2)
179-180: Update thesingleHashcomment to reflect the intended integration, not a pending fix.The comment still mentions “allow device to perform second sha256 hash … pending gridplus fix for hashType.NONE”, but
singleHash: trueis now the deliberate integration strategy for this signer mode rather than a temporary workaround. Reword the comment to describe whysingleHashis required with this device and drop the “pending fix” language.
183-191: Add THORChain vout-ordering validation before returning the PSBT-signed tx.The PSBT branch finalizes and returns the transaction without the
validateVoutOrderingcheck that other BTC implementations run whenmsg.vaultAddressis set, so THORChain vault sends could be signed even if the vout ordering is wrong.Recommend inserting this guard after extracting
tx:- const tx = psbt.extractTransaction(true); - - const signatures = psbt.data.inputs.map((input) => + const tx = psbt.extractTransaction(true); + + if (msg.vaultAddress && !core.validateVoutOrdering(msg, tx)) { + throw new Error("Invalid vout ordering for THORChain vault transaction"); + } + + const signatures = psbt.data.inputs.map((input) => input.partialSig ? Buffer.from(input.partialSig[0].signature).toString("hex") : "" );to fail fast on misordered THORChain transactions, matching the other wallet backends.
🧹 Nitpick comments (1)
packages/hdwallet-gridplus/src/bitcoin.ts (1)
151-176: Signature assembly looks correct; consider caching pubkeys to avoid extra device calls.Constructing the 64‑byte signature as
r || smatches what the PSBT signer expects, and the GridPlus SDK already padsrandsto 32 bytes, so no extra padding is needed. You are, however, fetching the public key from the device once when building the PSBT inputs and again here when creating the signer; caching the pubkey peraddressNListand reusing it would cut down on unnecessary hardware round‑trips.Based on learnings, the SDK automatically pads
randsto 32 bytes, so concatenation is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/hdwallet-gridplus/src/bitcoin.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
📚 Learning: 2025-11-20T11:04:44.808Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 737
File: packages/hdwallet-trezor/src/ethereum.ts:122-138
Timestamp: 2025-11-20T11:04:44.808Z
Learning: In packages/hdwallet-trezor/src/ethereum.ts, the ethSignTypedData function correctly returns the signature from res.payload.signature without adding a "0x" prefix. This works correctly in practice and has been tested, despite appearing inconsistent with ethSignMessage which does add the prefix. The Trezor Connect ethereumSignTypedData response already provides the signature in the correct format for consumption.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-10-15T23:22:26.842Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/thormaya.ts:99-105
Timestamp: 2025-10-15T23:22:26.842Z
Learning: In packages/hdwallet-gridplus/src/thormaya.ts, the GridPlus SDK (gridplus-sdk) automatically pads the r and s signature components to 32 bytes, so explicit padding in the code may be redundant but is not required. The thorchainSignTx implementation works without explicit padding because the SDK handles it.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-10-14T20:59:48.768Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 731
File: packages/hdwallet-gridplus/src/ethereum.ts:117-124
Timestamp: 2025-10-14T20:59:48.768Z
Learning: In packages/hdwallet-gridplus/src/ethereum.ts, the GridPlus SDK's sign method returns v as a number (integer), not a Buffer, so the Buffer.isBuffer(v) check is always false and vRaw = v is used directly.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:47:29.207Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/src/transport.ts:10-10
Timestamp: 2025-08-07T15:47:29.207Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the code works locally without TypeScript errors but has issues in CI environment. Using ts-expect-error would fail locally since there are no actual errors to suppress.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:47:26.835Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/src/transport.ts:12-12
Timestamp: 2025-08-07T15:47:26.835Z
Learning: In the shapeshiftoss/hdwallet monorepo, ts-ignore is used instead of ts-expect-error for Ledger transport imports because the CI environment has different type checking behavior than local development. The code works locally without errors, but CI reports type issues, so ts-ignore is necessary to suppress the inconsistent type checking across environments.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:24:19.530Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger-webusb/package.json:3-3
Timestamp: 2025-08-07T15:24:19.530Z
Learning: In the shapeshiftoss/hdwallet monorepo, the team runs `yarn build` before bumping versions and also before publishing packages. This ensures that dist/ artifacts (including UMD bundles) are properly regenerated after dependency updates.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:27:03.179Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-ledger/package.json:36-36
Timestamp: 2025-08-07T15:27:03.179Z
Learning: In the shapeshiftoss/hdwallet monorepo, the ledgerhq/hw-transport dependency in packages/hdwallet-ledger/package.json is pinned to an exact version (without caret) due to type mismatches that occur with newer versions. Other Ledger dependencies can safely use caret ranges.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
📚 Learning: 2025-08-07T15:23:54.858Z
Learnt from: gomesalexandre
Repo: shapeshift/hdwallet PR: 726
File: packages/hdwallet-coinbase/package.json:18-18
Timestamp: 2025-08-07T15:23:54.858Z
Learning: In the shapeshiftoss/hdwallet monorepo, package version bumps are done in PRs before publishing. The packages are published after the PR is merged, so dependency versions may reference unpublished versions during the PR review phase. This is expected behavior in their release workflow.
Applied to files:
packages/hdwallet-gridplus/src/bitcoin.ts
🧬 Code graph analysis (1)
packages/hdwallet-gridplus/src/bitcoin.ts (3)
packages/hdwallet-gridplus/src/utils.ts (1)
getCompressedPubkey(6-17)packages/hdwallet-gridplus/src/gridplus.ts (2)
btcGetAddress(422-425)btcSignTx(427-430)packages/hdwallet-core/src/bitcoin.ts (3)
BTCGetAddress(20-25)BTCSignTx(222-230)BTCSignedTx(237-242)
⏰ 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 and Release
🔇 Additional comments (2)
packages/hdwallet-gridplus/src/bitcoin.ts (2)
3-35: Address derivation helpers andbtcGetAddresslook solid.Centralizing network lookup + payment creation in
deriveAddressFromPubkey, validating the device pubkey ingetPublicKey, and handling coin-specific formatting inbtcGetAddressmakes the flow clear and robust; I don’t see any functional issues here.
38-89: Native GridPlus BTC signing path is correctly constrained and computed.The guard (
coin === "Bitcoin", single non-change output, noopReturnData) lines the native path up with what the device supports;recipient,fee(inputs − outputs), and the fallbackchangePathderived from the first input all look consistent and should keep the signer happy for standard single-recipient sends.
gomesalexandre
left a comment
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.
https://gist.github.com/gomesalexandre/ee79e9513e60a51c843ea3a015edc455
BTC branch happy now for Gridplus, UTXOs still fully borked
@kaladinlight after digging into this, looks like the implementation may be correct here for non-BTC UTXOs (still on it) but we may be missing patch in web, confirmed by applying there:
Assuming this hasn't been tested in the matching web PR - will stamp if/once I confirm applying the patch there works , as well as other wallets we need to regression test for, continuing testing with it.
gomesalexandre
left a comment
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.
See testing in gist: https://gist.github.com/gomesalexandre/ee79e9513e60a51c843ea3a015edc455
See patch in web: shapeshift/web@0b51495
Did a last paranoia check using upstream alpha instead of verdaccio bits for Gridplus (OP_RETURN / non-btc sends) and confirmed we're still gucci assuming patch is in in web:
gridplus.paranoia.mov
Resolved version conflicts by using 1.62.10 while preserving node_modules cleanup in clean scripts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
2dd058b to
1fb072e
Compare
effefae to
ea1b172
Compare
node_modulesonyarn cleanscriptsSummary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.