Feature: Add support for Nockster hardware wallet#54
Conversation
Greptile SummaryThis PR integrates the Nockster hardware wallet into Iris via the
Confidence Score: 3/5The core transaction flow is well-structured with correct cleanup on failure, but the fee estimation inconsistency and missing post-signing validation are worth addressing before shipping. The prepare/complete/cancel lifecycle correctly handles in-flight note cleanup. Two gaps stand out: estimateTransactionFee for Nockster queries the live RPC while prepareSendTransactionV2 uses the filtered UTXO store, which can produce a fee estimate that then fails at send time; and signRawTxWithNockster does not verify the device-returned signatures against the account public key the way signMessageWithNockster does, so a wrong-slot or corrupt signature would be broadcast and rejected by the network, temporarily locking input notes. extension/shared/vault.ts (estimateTransactionFee vs estimateMaxSendAmount UTXO source inconsistency) and extension/popup/utils/nockster.ts (signRawTxWithNockster missing signature verification).
|
| Filename | Overview |
|---|---|
| extension/popup/utils/nockster.ts | New 502-line file implementing Nockster device connection (HID/Serial), account listing, message signing with verification, and raw tx signing without signature verification. |
| extension/shared/vault.ts | Adds prepareSendTransactionV2/completePreparedSendTransactionV2/cancelPreparedSendTransactionV2; estimateTransactionFee uses live RPC for Nockster, inconsistent with estimateMaxSendAmount which uses the UTXO store. |
| extension/background/index.ts | Adds six new internal message handlers for the Nockster signing flow; completePendingRequest uses == instead of ===. |
| extension/shared/transaction-builder.ts | Refactors build functions into draft/signed variants; adds buildUnsignedTransaction and buildUnsignedMultiNotePayment for hardware wallet path with proper builder.free() cleanup. |
| extension/popup/screens/NocksterConnectScreen.tsx | New screen for HID/Serial pairing, PIN entry, account listing, and multi-account import as external seed sources. |
| extension/popup/screens/SendReviewScreen.tsx | Adds Nockster prepare-sign-complete send flow with correct cleanup on signing failure. |
| extension/popup/screens/approvals/TransactionApprovalScreen.tsx | Adds Nockster signing for dapp-initiated sends with correct cleanup on signing failure. |
| extension/popup/store.ts | Adds balanceError state, nockster-pair URL routing, SIGN_RAW_TX_HASH_PREFIX approval detection fix, and richer sync error propagation. |
| extension/shared/rpc-client-browser.ts | Fixes height parsing to handle bigint/string/number variants and removes the height=0 early return blocking balance reads. |
| extension/popup/screens/SendScreen.tsx | Adds Nockster-specific minimize fee toggle with proper fee re-estimation on change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User (Popup)
participant BG as Background (SW)
participant V as Vault
participant N as Nockster Device
note over U,N: Nockster Send Flow
U->>BG: PREPARE_SEND_TRANSACTION_V2
BG->>V: prepareSendTransactionV2(to, amount, fee, ...)
V->>V: getAvailableNotes (UTXO store)
V->>V: markNotesInFlight
V->>V: buildUnsignedMultiNotePayment
V-->>BG: "{ walletTx, rawTx, fee }"
BG-->>U: "{ walletTx, rawTx }"
U->>N: signRawTxWithNockster(rawTx, account)
N-->>U: signedTx (NockchainTx)
U->>BG: COMPLETE_SEND_TRANSACTION_V2 [walletTxId, signedTx]
BG->>V: completePreparedSendTransactionV2(walletTxId, signedTx)
V->>V: validate structure (no sig check)
V->>V: broadcast via RPC
V-->>BG: "{ txId, walletTx, broadcasted }"
BG-->>U: "{ txid, broadcasted }"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User (Popup)
participant BG as Background (SW)
participant V as Vault
participant N as Nockster Device
note over U,N: Nockster Send Flow
U->>BG: PREPARE_SEND_TRANSACTION_V2
BG->>V: prepareSendTransactionV2(to, amount, fee, ...)
V->>V: getAvailableNotes (UTXO store)
V->>V: markNotesInFlight
V->>V: buildUnsignedMultiNotePayment
V-->>BG: "{ walletTx, rawTx, fee }"
BG-->>U: "{ walletTx, rawTx }"
U->>N: signRawTxWithNockster(rawTx, account)
N-->>U: signedTx (NockchainTx)
U->>BG: COMPLETE_SEND_TRANSACTION_V2 [walletTxId, signedTx]
BG->>V: completePreparedSendTransactionV2(walletTxId, signedTx)
V->>V: validate structure (no sig check)
V->>V: broadcast via RPC
V-->>BG: "{ txId, walletTx, broadcasted }"
BG-->>U: "{ txid, broadcasted }"
Comments Outside Diff (2)
-
extension/popup/utils/nockster.ts, line 2133-2143 (link)Missing signature verification after device signing
signRawTxWithNocksterconverts the device-returned draft back into aNockchainTxbut never verifies that the embedded signatures are cryptographically valid for this account's public key. By contrast,signMessageWithNocksterexplicitly callswasm.verifySignatureand throws if verification fails.If the device signs with the wrong seed slot, returns a corrupt signature due to a firmware bug, or the
account.slotselection silently picks the wrong key, the resultingNockchainTxwill pass WASM structural checks and be submitted to the network. The network node will reject the invalid tx, and the input notes stay locked asbroadcasted_unconfirmeduntil the wallet's status-polling detects the permanent failure — temporarily freezing those funds for the user.Prompt To Fix With AI
This is a comment left during a code review. Path: extension/popup/utils/nockster.ts Line: 2133-2143 Comment: **Missing signature verification after device signing** `signRawTxWithNockster` converts the device-returned draft back into a `NockchainTx` but never verifies that the embedded signatures are cryptographically valid for this account's public key. By contrast, `signMessageWithNockster` explicitly calls `wasm.verifySignature` and throws if verification fails. If the device signs with the wrong seed slot, returns a corrupt signature due to a firmware bug, or the `account.slot` selection silently picks the wrong key, the resulting `NockchainTx` will pass WASM structural checks and be submitted to the network. The network node will reject the invalid tx, and the input notes stay locked as `broadcasted_unconfirmed` until the wallet's status-polling detects the permanent failure — temporarily freezing those funds for the user. How can I resolve this? If you propose a fix, please make it concise.
-
extension/shared/vault.ts, line 3225-3270 (link)estimateTransactionFeebypasses in-flight note filtering for Nockster accountsThe Nockster path in
estimateTransactionFeeissues a freshqueryV1BalanceRPC call, which returns all unspent notes regardless of whether they are already reserved by a concurrent prepare.estimateMaxSendAmountandprepareSendTransactionV2, on the other hand, both callthis.getAvailableNotes(), which excludes in-flight notes.The practical failure: fee estimation succeeds using a UTXO that is already locked by a pending prepare, but the subsequent
prepareSendTransactionV2call (which uses the filtered store) returns "Insufficient available funds" even though the estimated fee implied the transaction was viable. The user sees a confusing mismatch between the displayed fee and the send failure.Prompt To Fix With AI
This is a comment left during a code review. Path: extension/shared/vault.ts Line: 3225-3270 Comment: **`estimateTransactionFee` bypasses in-flight note filtering for Nockster accounts** The Nockster path in `estimateTransactionFee` issues a fresh `queryV1Balance` RPC call, which returns all unspent notes regardless of whether they are already reserved by a concurrent prepare. `estimateMaxSendAmount` and `prepareSendTransactionV2`, on the other hand, both call `this.getAvailableNotes()`, which excludes in-flight notes. The practical failure: fee estimation succeeds using a UTXO that is already locked by a pending prepare, but the subsequent `prepareSendTransactionV2` call (which uses the filtered store) returns "Insufficient available funds" even though the estimated fee implied the transaction was viable. The user sees a confusing mismatch between the displayed fee and the send failure. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
extension/popup/utils/nockster.ts:2133-2143
**Missing signature verification after device signing**
`signRawTxWithNockster` converts the device-returned draft back into a `NockchainTx` but never verifies that the embedded signatures are cryptographically valid for this account's public key. By contrast, `signMessageWithNockster` explicitly calls `wasm.verifySignature` and throws if verification fails.
If the device signs with the wrong seed slot, returns a corrupt signature due to a firmware bug, or the `account.slot` selection silently picks the wrong key, the resulting `NockchainTx` will pass WASM structural checks and be submitted to the network. The network node will reject the invalid tx, and the input notes stay locked as `broadcasted_unconfirmed` until the wallet's status-polling detects the permanent failure — temporarily freezing those funds for the user.
### Issue 2 of 4
extension/shared/vault.ts:3225-3270
**`estimateTransactionFee` bypasses in-flight note filtering for Nockster accounts**
The Nockster path in `estimateTransactionFee` issues a fresh `queryV1Balance` RPC call, which returns all unspent notes regardless of whether they are already reserved by a concurrent prepare. `estimateMaxSendAmount` and `prepareSendTransactionV2`, on the other hand, both call `this.getAvailableNotes()`, which excludes in-flight notes.
The practical failure: fee estimation succeeds using a UTXO that is already locked by a pending prepare, but the subsequent `prepareSendTransactionV2` call (which uses the filtered store) returns "Insufficient available funds" even though the estimated fee implied the transaction was viable. The user sees a confusing mismatch between the displayed fee and the send failure.
### Issue 3 of 4
extension/background/index.ts:1713-1737
**`COMPLETE_SIGN_MESSAGE` forwards `completeSignResponse` without shape validation**
`completeSignResponse` is taken directly from the message payload and passed verbatim to `completeSignPending.sendResponse()`, which forwards it to the requesting dapp provider. There is no check that the value conforms to the expected `SignMessageResponse` shape (`{ signature, publicKey }`). `COMPLETE_SIGN_RAW_TX` and `COMPLETE_TRANSACTION` both perform at least structural WASM validation before forwarding; this handler should do the same.
### Issue 4 of 4
extension/background/index.ts:248-253
Loose equality is used to compare string request IDs. Since both `currentRequestId` and `requestId` are strings (or `null`), strict equality is more correct and avoids any implicit coercion surprises.
```suggestion
function completePendingRequest(requestId: string): void {
pendingRequests.delete(requestId);
if (currentRequestId === requestId) {
currentRequestId = null;
}
}
```
Reviews (1): Last reviewed commit: "Fix HID and protocol exchange; add multi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| return; | ||
|
|
||
| case INTERNAL_METHODS.COMPLETE_SIGN_MESSAGE: | ||
| const [completeSignId, completeSignResponse] = payload.params || []; | ||
| const completeSignPending = pendingRequests.get(completeSignId); | ||
| if (completeSignPending && isSignRequest(completeSignPending.request)) { | ||
| const signRequest = completeSignPending.request; | ||
| if (isRequestExpired(signRequest.timestamp)) { | ||
| cancelPendingRequest(completeSignId, 4003, 'Request expired'); | ||
| sendResponse({ error: 'Request expired' }); | ||
| return; | ||
| } | ||
| completeSignPending.sendResponse(completeSignResponse); | ||
| completePendingRequest(completeSignId); | ||
| processNextRequest(); | ||
| sendResponse({ success: true }); | ||
| } else { | ||
| sendResponse({ error: ERROR_CODES.NOT_FOUND }); | ||
| } | ||
| return; | ||
|
|
||
| case INTERNAL_METHODS.APPROVE_SIGN_RAW_TX: | ||
| const approveSignRawTxId = payload.params?.[0]; | ||
| const approveSignRawTxPending = pendingRequests.get(approveSignRawTxId); |
There was a problem hiding this comment.
COMPLETE_SIGN_MESSAGE forwards completeSignResponse without shape validation
completeSignResponse is taken directly from the message payload and passed verbatim to completeSignPending.sendResponse(), which forwards it to the requesting dapp provider. There is no check that the value conforms to the expected SignMessageResponse shape ({ signature, publicKey }). COMPLETE_SIGN_RAW_TX and COMPLETE_TRANSACTION both perform at least structural WASM validation before forwarding; this handler should do the same.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extension/background/index.ts
Line: 1713-1737
Comment:
**`COMPLETE_SIGN_MESSAGE` forwards `completeSignResponse` without shape validation**
`completeSignResponse` is taken directly from the message payload and passed verbatim to `completeSignPending.sendResponse()`, which forwards it to the requesting dapp provider. There is no check that the value conforms to the expected `SignMessageResponse` shape (`{ signature, publicKey }`). `COMPLETE_SIGN_RAW_TX` and `COMPLETE_TRANSACTION` both perform at least structural WASM validation before forwarding; this handler should do the same.
How can I resolve this? If you propose a fix, please make it concise.| function completePendingRequest(requestId: string): void { | ||
| pendingRequests.delete(requestId); | ||
| if (currentRequestId == requestId) { | ||
| currentRequestId = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
currentRequestId and requestId are strings (or null), strict equality is more correct and avoids any implicit coercion surprises.
| function completePendingRequest(requestId: string): void { | |
| pendingRequests.delete(requestId); | |
| if (currentRequestId == requestId) { | |
| currentRequestId = null; | |
| } | |
| } | |
| function completePendingRequest(requestId: string): void { | |
| pendingRequests.delete(requestId); | |
| if (currentRequestId === requestId) { | |
| currentRequestId = null; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extension/background/index.ts
Line: 248-253
Comment:
Loose equality is used to compare string request IDs. Since both `currentRequestId` and `requestId` are strings (or `null`), strict equality is more correct and avoids any implicit coercion surprises.
```suggestion
function completePendingRequest(requestId: string): void {
pendingRequests.delete(requestId);
if (currentRequestId === requestId) {
currentRequestId = null;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
added nockster hardware wallet support to iris using the
@swps/nockster-jsnode libsignDraftflow; iris sends the unsigned noun over the wire1.3.0