feat: hdwallet-gridplus add SafeCard validation - #748
Conversation
Implements three-point validation for GridPlus SafeCards: 1. Initial pairing - capture wallet UID 2. Reconnection - validate correct SafeCard inserted 3. Just-in-time before signing - prevent wrong SafeCard swaps Changes: - Add validateActiveWallet() to fetch and validate wallet UID - Add setExpectedWalletUid() for JIT validation setup - Add validateBeforeSigning() called before all signing methods - Update adapter pairDevice() to return walletUid and isExternal - Update adapter connectDevice() to accept expectedWalletUid - Wire validation to: ETH, BTC, Solana, Cosmos, Thorchain, Mayachain Note: Built on top of a1a7c01 which has type errors in bitcoin.ts Need to fix base commit type errors before this can build.
📝 WalkthroughWalkthroughGridPlus adapter and wallet code now surface and validate SafeCard active-wallet metadata. connectDevice/pairDevice signatures and returns changed to include activeWalletId and type. GridPlusHDWallet stores expected active-wallet info and enforces validation before pairing and all signing operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Adapter as GridPlus Adapter
participant Device as GridPlus Device
participant Wallet as GridPlusHDWallet
Note over Client,Adapter: Pairing returns wallet + active metadata
Client->>Adapter: pairDevice(pairingCode)
Adapter->>Device: establish pairing/session
Device-->>Adapter: pairing success
Adapter->>Device: query active wallet data
Device-->>Adapter: { activeWalletId, type }
Adapter-->>Client: { wallet, activeWalletId, type }
Note over Client,Adapter: Connect with expectations
Client->>Adapter: connectDevice(deviceId, expectedActiveWalletId?, expectedType?)
Adapter->>Wallet: setExpectedActiveWalletId(...)
Wallet->>Device: query active wallet
Wallet-->>Adapter: { activeWalletId, type } or Error
Adapter-->>Client: GridPlusHDWallet or undefined/error
Note over Client,Wallet: Signing requires validation
Client->>Wallet: ethSignTx(tx)
Wallet->>Wallet: validateActiveWallet(expectedActiveWalletId, expectedType)
alt validated
Wallet->>Device: Sign tx
Device-->>Wallet: signature
Wallet-->>Client: signed result
else validation failed
Wallet-->>Client: Error (active wallet mismatch)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (27)
🚧 Files skipped from review as they are similar to previous changes (23)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-08-07T15:27:03.179ZApplied to files:
📚 Learning: 2025-08-07T15:23:54.858ZApplied to files:
📚 Learning: 2025-08-07T15:24:19.530ZApplied to files:
📚 Learning: 2025-08-07T15:47:29.207ZApplied to files:
📚 Learning: 2025-08-07T15:47:26.835ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
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 |
SafeCards are the base case, internal wallet is the edge case. Updated naming to reflect this throughout.
- Changed isInternal boolean to type: 'external' | 'internal' - Simplified validateActiveWallet logic using type to index activeWallets - Removed all console.log statements - Much cleaner diff\!
Calling validateActiveWallet() after pair() caused Device Locked error because it triggered a second fetchActiveWallet() call while device was still processing the first one from SDK's pair() function. Fixed by using client.getActiveWallet() which returns cached data.
Renamed across GridPlus adapter and wallet implementation: - walletUid → activeWalletId - expectedWalletUid → expectedActiveWalletId - setExpectedWalletUid → setExpectedActiveWalletId - uid field in return types → activeWalletId Also bumped to 1.62.13-gridplus-validation.12 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
During initial pairing flow, connectDevice() was being called twice: 1. From Connect screen (not yet paired) 2. From Pair screen (after entering pairing code) The second call would hit the else branch and call resetActiveWallets(), but this was unnecessary during pairing and could interfere with the cache populated by pair(). Removed the resetActiveWallets() call from connectDevice(). Cache clearing is not needed during normal pairing flow - only for reconnection/switching scenarios which will be handled separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Previous commit incorrectly removed this - it's needed for proper cache management when client already exists. The Device Locked error was actually caused by duplicate connectDevice() calls in web utils, not by this resetActiveWallets() call. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
gomesalexandre
changed the title
When expectedType is 'internal' and validation fails, throw more specific error: "Remove inserted SafeCard to access internal GridPlus wallet" This allows web to show different error messages for: - External SafeCard mismatch: "Wrong SafeCard inserted..." - Internal wallet blocked by external card: "Remove inserted SafeCard..." Changes: - Added expectedType property to track expected wallet type - Updated setExpectedActiveWalletId() to accept optional type parameter - Throw different error messages based on expected type 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replaced direct Buffer.equals(ZERO_BUFFER) calls with semantic isEmptyWallet() helper function for better readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replaced ZERO_BUFFER allocation with buffer.every(b => b === 0) for more efficient zero-check without allocating comparison buffer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Combined improvements: - Optimized buffer comparison using buffer.every() instead of allocating ZERO_BUFFER - Added expectedType property for context-aware error messages - setExpectedActiveWalletId() now accepts optional type parameter - Throws specific errors: - Internal: "Remove inserted SafeCard to access internal GridPlus wallet" - External: "Active SafeCard doesn't match expected SafeCard" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated validateActiveWallet() and connectDevice() signatures to accept expectedType parameter, and pass it through from all signing methods. This ensures the correct context-aware error message is thrown based on whether internal or external wallet is expected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Reverted all package.json and lerna.json files to origin/master to keep diff focused on actual code changes only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Keep diff focused only on GridPlus validation changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Re-added comment from master explaining why resetActiveWallets is called when client already exists. 🤖 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>
TypeScript infers the type from the ternary expression. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Created SafeCardType = 'external' | 'internal' to reduce repetition and improve maintainability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The walletName field was never used and is always empty anyway. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replaced buffer.every() with hex string comparison against viem's zeroHash constant for cleaner and more semantic code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
gomesalexandre
changed the title
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ment - Renamed to isZeroSafecardUid for better clarity - Updated validateActiveWallet JSDoc to reflect current implementation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Positive logic (isSafecardConnected) is clearer than double negative (!isSafecardDisconnected). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changed from silently skipping validation to throwing an error if expectedActiveWalletId is not set. This catches bugs where we forget to call setExpectedActiveWalletId() after wallet setup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/hdwallet-gridplus/src/gridplus.ts (1)
476-478: Consider extracting repeated validation pattern.The validation pattern (check
expectedActiveWalletId, callvalidateActiveWallet) is repeated identically across all signing methods (lines 476-478, 497-499, 505-507, 513-515, 542-544, 555-557, 568-570, 581-583). While functional, this creates maintenance overhead.Consider extracting to a private helper:
private async ensureValidatedWallet(): Promise<void> { if (!this.expectedActiveWalletId) throw new Error("Expected SafeCard ID not set"); await this.validateActiveWallet(this.expectedActiveWalletId, this.expectedType); }Then use in signing methods:
async btcSignTx(msg: core.BTCSignTx): Promise<core.BTCSignedTx | null> { if (!this.client) throw new Error("Device not connected"); - if (!this.expectedActiveWalletId) throw new Error("Expected SafeCard ID not set"); - - await this.validateActiveWallet(this.expectedActiveWalletId, this.expectedType); + await this.ensureValidatedWallet(); return btc.btcSignTx(this.client, msg); }
📜 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 (2)
packages/hdwallet-gridplus/src/adapter.ts(4 hunks)packages/hdwallet-gridplus/src/gridplus.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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/adapter.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/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: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-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/adapter.ts (1)
packages/hdwallet-gridplus/src/gridplus.ts (1)
GridPlusHDWallet(275-586)
🔇 Additional comments (15)
packages/hdwallet-gridplus/src/adapter.ts (4)
34-40: Good practice: Resetting active wallets on reconnect.The explicit call to
resetActiveWallets()when reusing an existing client is well-commented and ensures fresh wallet state from the device, which is critical for SafeCard switching scenarios.
42-50: LGTM: Clean validation flow.The optional validation pattern allows backward compatibility while enforcing SafeCard validation when required. The early return on pairing failure and conditional validation logic are well-structured.
30-32: I'll help you verify the impact of removing the password from the hash input. Let me investigate the codebase to understand the context and confirm whether this is truly a breaking change.
<function_calls>
#!/bin/bashFirst, let's examine the file structure and find the adapter.ts file
fd "adapter.ts" -t f
Get file size to determine how to read it
wc -l packages/hdwallet-gridplus/src/adapter.ts 2>/dev/null || echo "File not found in initial search"
</function_calls>
<function_calls>
#!/bin/bashRead the adapter.ts file around the lines in question (25-50)
cat -n packages/hdwallet-gridplus/src/adapter.ts | sed -n '20,55p'
</function_calls>
<function_calls>
#!/bin/bashSearch for git history showing what the previous version looked like
cd /tmp && git clone --depth=1 https://github.com/shapeshift/hdwallet.git 2>/dev/null || echo "Clone check"
cd packages/hdwallet-gridplus 2>/dev/null || echo "Not in cloned repo"Get the full history of the specific file to see if password was previously included
git log --oneline packages/hdwallet-gridplus/src/adapter.ts 2>/dev/null | head -20
</function_calls>
<function_calls>
#!/bin/bashSearch for password-related patterns in the adapter file
rg -nC5 "password" packages/hdwallet-gridplus/src/adapter.ts
</function_calls>
<function_calls>
#!/bin/bashSearch for migration logic or breaking change documentation
rg -nC3 "migration|re-pair|re-pairing|breaking change|deviceId.*password" --type=ts packages/hdwallet-gridplus/
</function_calls>
<function_calls>
#!/bin/bashLook at the connectDevice method signature and surrounding context more carefully
cat -n packages/hdwallet-gridplus/src/adapter.ts | sed -n '1,100p'
</function_calls>
65-70: The code's null check is correct. GridPlus SDK'sClient.getActiveWallet()returnsnullorundefinedwhen no wallet is active, not an object with undefined properties.According to GridPlus SDK documentation,
getActiveWallet()returns an object containing:
uid— 32-byte Buffername— stringcapabilities— 4-byte flagexternal— booleanThe null check at line 66 (
if (!activeWallet)) properly handles the case when no wallet is active, and the subsequent property access is safe.packages/hdwallet-gridplus/src/gridplus.ts (11)
15-15: LGTM: Clear SafeCard detection logic.The
isSafecardConnectedhelper correctly identifies active SafeCards by checking for non-zero UIDs. The use ofzeroHashfrom viem provides a well-known constant for comparison.
305-306: LGTM: Appropriate state management.The private optional fields for tracking expected wallet ID and type are well-scoped and align with the validation workflow introduced in this PR.
313-316: LGTM: Simple setter for expected wallet state.The public setter allows the adapter to configure expected wallet credentials after pairing or connecting, enabling subsequent validation during signing operations.
375-382: LGTM: Simplified with helper function.The refactored
getActiveWalletIdcorrectly uses theisSafecardConnectedhelper to determine which wallet (external prioritized over internal) is active.
388-419: LGTM: Robust validation with clear error messages.The
validateActiveWalletmethod implements thorough SafeCard validation with appropriate error handling:
- Prioritizes external over internal wallets (line 401-402)
- Provides context-specific error messages for internal wallet access vs. SafeCard mismatch (lines 412-415)
- Returns validated wallet details for downstream use
495-500: Validation pattern already covered in previous comment for btcSignTx.
503-516: Validation pattern already covered in previous refactor suggestion.
523-533: LGTM: Appropriate firmware version check.The
assertSolanaFwSupportmethod uses a TypeScript assertion function to ensure firmware compatibility before Solana operations. The version check (>= 0.14.0) and clear error message are well-implemented.
540-545: Validation pattern already covered in previous refactor suggestion.
553-585: Validation patterns for cosmosSignTx, thorchainSignTx, and mayachainSignTx already covered in previous refactor suggestion.
4-4: Add viem to package dependencies in packages/hdwallet-gridplus/package.json.The code imports
zeroHashfrom viem at line 4, but viem is not declared as a dependency. This will cause a runtime error. viem v2.40.3 has no known security vulnerabilities, but it must be added to the package.json dependencies before this code can function. Verify the appropriate version constraint (e.g.,^2.40.3) and add it to both dependencies and any peer dependency declarations as needed.⛔ Skipped due to learnings
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.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.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.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.
Resolved conflicts by taking master's version of all package.json and lerna.json files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (27)
examples/sandbox/package.json(2 hunks)integration/package.json(2 hunks)lerna.json(1 hunks)packages/hdwallet-coinbase/package.json(2 hunks)packages/hdwallet-core/package.json(2 hunks)packages/hdwallet-gridplus/package.json(3 hunks)packages/hdwallet-keepkey-chromeusb/package.json(2 hunks)packages/hdwallet-keepkey-electron/package.json(3 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(3 hunks)packages/hdwallet-ledger-webhid/package.json(2 hunks)packages/hdwallet-ledger-webusb/package.json(2 hunks)packages/hdwallet-ledger/package.json(3 hunks)packages/hdwallet-metamask-multichain/package.json(3 hunks)packages/hdwallet-native-vault/package.json(3 hunks)packages/hdwallet-native/package.json(3 hunks)packages/hdwallet-phantom/package.json(3 hunks)packages/hdwallet-portis/package.json(3 hunks)packages/hdwallet-trezor-connect/package.json(2 hunks)packages/hdwallet-trezor/package.json(3 hunks)packages/hdwallet-vultisig/package.json(3 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 (16)
- lerna.json
- packages/hdwallet-native-vault/package.json
- packages/hdwallet-vultisig/package.json
- packages/hdwallet-trezor/package.json
- packages/hdwallet-portis/package.json
- packages/hdwallet-coinbase/package.json
- packages/hdwallet-keplr/package.json
- packages/hdwallet-phantom/package.json
- packages/hdwallet-gridplus/package.json
- packages/hdwallet-keepkey-webusb/package.json
- packages/hdwallet-keepkey/package.json
- packages/hdwallet-keepkey-nodehid/package.json
- packages/hdwallet-walletconnect/package.json
- examples/sandbox/package.json
- packages/hdwallet-native/package.json
- packages/hdwallet-keepkey-tcp/package.json
🧰 Additional context used
🧠 Learnings (6)
📓 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-keepkey-chromeusb/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-ledger-webusb/package.jsonintegration/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.jsonpackages/hdwallet-core/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-keepkey-chromeusb/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-ledger-webusb/package.jsonintegration/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.jsonpackages/hdwallet-core/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-keepkey-chromeusb/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-ledger-webusb/package.jsonintegration/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.jsonpackages/hdwallet-core/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-keepkey-chromeusb/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-ledger-webusb/package.jsonintegration/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.jsonpackages/hdwallet-core/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-keepkey-chromeusb/package.jsonpackages/hdwallet-ledger-webhid/package.jsonpackages/hdwallet-ledger/package.jsonpackages/hdwallet-ledger-webusb/package.jsonintegration/package.jsonpackages/hdwallet-metamask-multichain/package.jsonpackages/hdwallet-walletconnectV2/package.jsonpackages/hdwallet-trezor-connect/package.jsonpackages/hdwallet-keepkey-electron/package.jsonpackages/hdwallet-keepkey-nodewebusb/package.json
⏰ 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 (13)
integration/package.json (1)
3-3: Version and dependency synchronization consistent across integration package.All hdwallet-related dependencies have been updated to the new prerelease version
^1.62.16-bro-seriously-fml.0in alignment with the coordinated version bump across the monorepo.Also applies to: 14-23
packages/hdwallet-ledger/package.json (1)
3-3: Version and gitHead metadata properly synchronized.Package version bumped to prerelease identifier and gitHead field added consistently with the monorepo's release pattern. Core dependency properly updated to match.
Also applies to: 28-28, 47-47
packages/hdwallet-core/package.json (1)
3-3: Core package version and metadata updated consistently.Version and gitHead fields properly synchronized as part of the coordinated prerelease.
Also applies to: 31-31
packages/hdwallet-walletconnectV2/package.json (1)
3-3: WalletConnectV2 adapter synchronized with core version bump.Version, core dependency, and gitHead metadata all properly updated.
Also applies to: 18-18, 23-23
packages/hdwallet-keepkey-chromeusb/package.json (1)
3-3: KeepKey ChromeUSB transport dependencies properly synchronized.Both core and keepkey adapter dependencies updated to prerelease version with consistent gitHead metadata.
Also applies to: 17-18, 20-20
packages/hdwallet-metamask-multichain/package.json (1)
3-3: Metamask multichain adapter version and dependencies synchronized.Version, core dependency, and gitHead consistent with monorepo prerelease pattern.
Also applies to: 20-20, 31-31
packages/hdwallet-keepkey-nodewebusb/package.json (1)
3-3: KeepKey NodeWebUSB transport dependencies properly versioned.Both core and keepkey adapter dependencies updated to prerelease version with consistent gitHead metadata.
Also applies to: 17-18, 26-26
packages/hdwallet-trezor-connect/package.json (2)
3-3: Trezor Connect adapter version and dependencies synchronized.Both core and trezor adapter dependencies updated to prerelease version with consistent gitHead metadata.
Also applies to: 17-18, 21-21
1-22: Verify that build artifacts have been regenerated.Per your team's workflow, ensure that
yarn buildhas been executed across all modified packages to regenerate dist/ artifacts and TypeScript build info before finalizing this PR.packages/hdwallet-ledger-webusb/package.json (1)
23-24: Verify caret ranges are appropriate for pre-release dependency versions.Caret version ranges (
^1.62.16-bro-seriously-fml.0) may not behave predictably with pre-release versions during dependency resolution. Confirm that the dependency resolution aligns with your intended version constraints.packages/hdwallet-ledger-webhid/package.json (2)
25-25: Added gitHead metadata.The new
gitHeadfield follows the monorepo's standard publishing pattern and appears appropriate.
3-3: Based on my verification, the original review comment is completely justified and reveals a critical issue. Here's what I found:Key findings:
Monorepo-wide problem: The version string "1.62.16-bro-seriously-fml.0" appears in all 19 package.json files plus lerna.json (the monorepo root configuration)
Systematic, not accidental: This is not a typo in one file—it's consistent across:
- All core packages (hdwallet-core, hdwallet-ledger, hdwallet-ledger-webhid, etc.)
- All wallet adapters (Trezor, KeepKey, Keplr, etc.)
- Integration tests and examples
- Dependency versions are synchronized to match
Auto-generated by build process: The git commit message states "chore: update internal dependencies after prepublish build"—indicating this version was set by an automated monorepo versioning tool (Lerna) during a build step
Will be published publicly: The package.json includes
"publishConfig": { "access": "public" }, confirming this is destined for the npm public registry where this version string will be permanently visibleThis is a critical issue that must be resolved before merging or publishing.
The version string "1.62.16-bro-seriously-fml.0" is set across the entire monorepo and will be published to npm if this PR is merged. This must be corrected to a valid semantic version (e.g., "1.62.16") before release.
All 19 packages in
packages/,examples/, andintegration/directories, plus the lerna.json root config, contain this version. Verify the automated versioning process and ensure it generates production-appropriate version strings.⛔ Skipped due to learnings
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.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.packages/hdwallet-keepkey-electron/package.json (1)
3-3: Version naming is intentional and consistent across the entire monorepo—not a debug artifact.The verification confirms that "1.62.16-bro-seriously-fml.0" is applied uniformly to all 26 package.json files in the monorepo (not just keepkey-electron). The gitHead field (50218b2) is also consistently added across 24+ packages. This is a coordinated monorepo-wide version bump and metadata standardization, not isolated to a single package. The commit message "chore: update internal dependencies after prepublish build" indicates this follows the team's prepublish workflow. While the version string's informal tone is unconventional, it is semver-compliant and intentional across the entire release.
|
Going to yeet this, rationale:
|
NeOMakinG
left a comment
NeOMakinG
left a comment
There was a problem hiding this comment.
Can't test as I'm inside a car right now but code looks sane
2 participants
Description
Test me with the matching web PR
This PR:
Screenshots
gridplus.validation.final.testing.mov
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.