docs: Fullnode Typings [11] - Connection and Websockets#1013
docs: Fullnode Typings [11] - Connection and Websockets#1013
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1013 +/- ##
==========================================
- Coverage 87.38% 87.35% -0.04%
==========================================
Files 114 114
Lines 8750 8760 +10
Branches 1963 1970 +7
==========================================
+ Hits 7646 7652 +6
- Misses 1076 1080 +4
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| getState(): ConnectionState { | ||
| return this.state; | ||
| } |
There was a problem hiding this comment.
All code used to access state directly up until now. With Typescript this direct access is no longer permitted, so a getter method was created.
| /** | ||
| * TODO: This should be removed when this file is migrated to typescript | ||
| * we need this here because the typescript enum from the Connection file is | ||
| * not being correctly transpiled here, returning `undefined` for ConnectionState.CLOSED. | ||
| */ | ||
| const ConnectionState = { | ||
| CLOSED: 0, | ||
| CONNECTING: 1, | ||
| CONNECTED: 2, | ||
| }; |
| if (!pinCode) { | ||
| throw new Error(ERROR_MESSAGE_PIN_REQUIRED); | ||
| } | ||
| if (!password) { | ||
| throw new Error(ERROR_MESSAGE_PASSWORD_REQUIRED); | ||
| } |
There was a problem hiding this comment.
Guarding the types of the variables that are used below
There was a problem hiding this comment.
It is a type guard but also a new error case.
We should check that the wallets don't expect some special case where pin or password are not passed (shouldn't exist this case, but we should check anyway).
There was a problem hiding this comment.
★ Insight ─────────────────────────────────────
The HathorWallet.start() method has a subtle design: pinCode and password can be provided either at construction time or at start time (the start() options override the constructor values). The start() method falls back to this.pinCode / this.password if not passed. This means a call like wallet.start() with no args is valid only if credentials were already supplied in the constructor.
─────────────────────────────────────────────────
Here's the full summary across all four repos:
Results
| Repository | File | pinCode | password | Notes |
|---|---|---|---|---|
| hathor-rpc-lib | packages/snap/src/utils/wallet.ts:48 |
DEFAULT_PIN_CODE |
DEFAULT_PIN_CODE |
Also passes waitReady: false |
| hathor-rpc-lib | packages/snap/src/utils/wallet.ts:143 |
DEFAULT_PIN_CODE |
DEFAULT_PIN_CODE |
|
| hathor-wallet | src/sagas/wallet.js:236 |
pin |
password |
Via redux-saga yield call() |
| hathor-wallet-headless | src/services/wallets.service.js:140 |
MISSING | MISSING | wallet.start() with no args |
| hathor-wallet-mobile | src/sagas/wallet.js:261 |
pin |
pin |
Same value for both |
| hathor-wallet-mobile | src/sagas/pushNotification.js:412 |
pin |
pin |
Push notification wallet service |
Key Finding
hathor-wallet-headless is the only consumer that calls wallet.start() with no arguments. However, this is safe because:
- The headless wallet passes
pinCodeandpassword(orseed/xpub) at constructor time viahydratedWalletConfig, not atstart()time. - The
start()method falls back tothis.pinCodeandthis.password(lines 1611-1612 ofwallet.ts):const pinCode = options.pinCode || this.pinCode; const password = options.password || this.password;
- So it would only throw if both the constructor AND
start()omit the credentials.
That said, for xpub-only (read-only) wallets, pinCode is not required (!this.xpub && !pinCode is the check on line 1613), so the headless wallet's xpub path is also safe.
Impact of adding error handling: Your changes should be backwards-compatible with all consumers since they all either pass credentials at start() time or at constructor time. The headless wallet is the one to watch — just make sure the fallback to this.pinCode/this.password is preserved before any new throws.
There was a problem hiding this comment.
★ Insight ─────────────────────────────────────
The headless wallet uses a "default credentials" pattern — both getReadonlyWalletConfig and getWalletConfigFromSeed always inject DEFAULT_PASSWORD and DEFAULT_PIN into the config object (lines 72-73 and 105-106). These are passed to the HathorWallet constructor (not to .start()), so when wallet.start() is called with no args, the start() method falls back to this.pinCode / this.password which were set during construction.
─────────────────────────────────────────────────
Here's the full trace for all three controllers:
Index Controller (index.controller.js)
Two paths, both safe:
- xpub path (line 165): calls
getReadonlyWalletConfig({ xpub, ... })→ setspinCode: DEFAULT_PIN,password: DEFAULT_PASSWORD - seed/seedKey path (line 200): calls
getWalletConfigFromSeed({ seed, ... })→ setspinCode: DEFAULT_PIN,password: DEFAULT_PASSWORD
Both configs are passed to new HathorWallet(hydratedWalletConfig) at construction. Then wallet.start() on line 140 passes no args, but start() falls back to this.pinCode / this.password from the constructor. No crack.
HSM Controller (hsm.controller.js)
- Line 119: calls
getReadonlyWalletConfig({ xpub: xPub })→ setspinCode: DEFAULT_PIN,password: DEFAULT_PASSWORD - Line 125: calls
startWallet(walletId, walletConfig, config, { hsmKeyName })
Same pattern — credentials are in the constructor config. No crack.
Fireblocks Controller (fireblocks.controller.js)
- Line 107: calls
getReadonlyWalletConfig({ xpub })→ setspinCode: DEFAULT_PIN,password: DEFAULT_PASSWORD - Line 110: calls
startWallet(walletId, walletConfig, config)
Same pattern. No crack.
The Safety Net: wallet.helper.js
Both helper functions (getReadonlyWalletConfig at line 63, getWalletConfigFromSeed at line 81) always hardcode credentials:
// wallet.helper.js:70-73
const walletConfig = {
xpub,
password: DEFAULT_PASSWORD, // always set
pinCode: DEFAULT_PIN, // always set
...
};With a comment explicitly noting: "Previous versions of the lib would have password and pin default as '123'. We currently need something to be defined, otherwise we get an error when starting the wallet." ( link to code )
Verdict
All three controllers are safe. Every path through the headless wallet passes pinCode and password at constructor time via the wallet helper functions. The wallet.start() call with no args works fine because it falls back to this.pinCode and this.password. Your error handling changes in start() won't break any headless wallet flows.
There was a problem hiding this comment.
TL;DR: No backwards compatibility broken: this code is safe.
| statements: 92, | ||
| branches: 85, | ||
| statements: 90, | ||
| branches: 83, | ||
| functions: 90, | ||
| lines: 92, | ||
| lines: 90, |
There was a problem hiding this comment.
I understand that most of the code changed did not have tests initially, but I don't think we should lower the standards without an approval from @pedroferreira1
Maybe we can add some tests for the new error cases to increase the line coverage?
There was a problem hiding this comment.
The main objective for these PRs is not to change code logic, so adding more tests are not within this scope here, precisely for us to feel safer when having to change settings like this one.
With the amount of lines that I'm changing, those coverage bars are sure to be changed. I agree we should wait for Pedro on this one.
| if (!pinCode) { | ||
| throw new Error(ERROR_MESSAGE_PIN_REQUIRED); | ||
| } | ||
| if (!password) { | ||
| throw new Error(ERROR_MESSAGE_PASSWORD_REQUIRED); | ||
| } |
There was a problem hiding this comment.
It is a type guard but also a new error case.
We should check that the wallets don't expect some special case where pin or password are not passed (shouldn't exist this case, but we should check anyway).
| obj !== null && | ||
| typeof obj === 'object' && | ||
| 'type' in obj && | ||
| typeof (obj as { type: unknown }).type === 'string'; |
There was a problem hiding this comment.
I was thinking we could do this in another way.
The wsTxQueue is a Queue which when this was a js file did not have typing but we could add now.
The Queue already uses generic typing, so we could define it as Queue[WalletWebSocketData] and let the type checker validate the typing when enqueueing and dequeueing.
It would make this isWalletWebSocketData unnecessary (since the dequeue would get a typed wsData) and it would make all places adding data to the queue check that type as well
There was a problem hiding this comment.
Thanks for the idea! Implemented on 75f9196 , with some unit test changes to match the stricter typing.
| await addCreatedTokenFromTx(transaction as CreateTokenTransaction, storage); | ||
| // Add new transaction to the wallet's storage. | ||
| wallet.enqueueOnNewTx({ history: historyTx }); | ||
| wallet.enqueueOnNewTx({ type: '', history: historyTx }); // FIXME: Add a type here |
There was a problem hiding this comment.
The type expected from the fullnode is 'wallet:address_history'.
But I think we can change it to lib:pushed-tx to mark it as a wallet-lib generated event and that it came from pushing a transaction to the network.
This is because we want to update the balance, mark utxos as spent as soon as the tx is sent, this makes it possible to send another transaction (and choose utxos for this new tx safely) faster.
There was a problem hiding this comment.
Since i'm just adding types and not changing code, I'd prefer leaving the type as empty and the fixme command.
The correct behavior here would be to enqueueOnNewTx reject the missing type, and both those elements reinforce that it needs attention. There are many other similar "FixMe" clauses on those fullnode typings prs with that intention.
| connection: { state: ConnectionState.CONNECTED }, | ||
| connection: { | ||
| state: ConnectionState.CONNECTED, | ||
| getState(): ConnectionState { |
There was a problem hiding this comment.
since you already added the getState in the connectionFile. What about use it here?
There was a problem hiding this comment.
This is a mock object to test the wallet start edge cases without actually connecting.
This should be moved to the unit tests suite later: there should be no mocks on integration tests.
raul-oliveira
left a comment
raul-oliveira
left a comment
There was a problem hiding this comment.
LGTM. Just a comment about the connection object
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds stronger typing across wallet code, makes Connection.getState() public, widens storage signature to accept null tx-signing function, changes wallet to use connection.getCurrentNetwork(), adjusts enqueue payloads for pushed transactions, and updates tests and Jest coverage thresholds accordingly. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/new/hathorwallet.test.ts`:
- Around line 397-401: The test uses an unsafe double-cast to supply websocket
history as an array; replace the enqueue payload with a properly shaped
IHistoryTx object so onNewTx's IHistoryTxSchema.safeParse(wsData.history)
receives the expected single-object shape. Locate the test code setting
hWallet.wsTxQueue.enqueue and change the history property to a valid IHistoryTx
instance (matching the IHistoryTx type/fields used by IHistoryTxSchema) instead
of `[1] as unknown as IHistoryTx`, ensuring WalletWebSocketData.history conforms
to IHistoryTx.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__tests__/integration/hathorwallet_facade.test.ts__tests__/new/hathorwallet.test.tsjest-integration.config.jssrc/connection.tssrc/new/sendTransaction.tssrc/new/wallet.ts
| hWallet.wsTxQueue = new Queue<WalletWebSocketData>(); | ||
| hWallet.wsTxQueue.enqueue({ | ||
| type: 'wallet:address_history', | ||
| history: [1] as unknown as IHistoryTx, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if the files exist and their basic structure
echo "=== Checking file existence ==="
git ls-files | grep -E "(types\.ts|wallet\.ts|hathorwallet\.test\.ts)"
echo -e "\n=== Searching for WalletWebSocketData interface ==="
rg -n -C4 'interface WalletWebSocketData|history\?' src/new/types.ts 2>/dev/null || echo "Pattern not found or file doesn't exist in src/new/"
echo -e "\n=== Searching for websocket handling patterns ==="
rg -n -C4 'handleWebsocketMsg|onNewTx|wsTxQueue|wallet:address_history|history' src/new/wallet.ts __tests__/new/hathorwallet.test.ts 2>/dev/null || echo "Patterns not found or files don't exist"Repository: HathorNetwork/hathor-wallet-lib
Length of output: 20607
Use the correct shape for websocket history in test fixtures instead of unsafe double-cast.
The interface defines history?: IHistoryTx as a single object, but the test passes [1] as unknown as IHistoryTx (an array). This double-cast masks a real type mismatch. The onNewTx method validates wsData.history with IHistoryTxSchema.safeParse(wsData.history), expecting a single object. Update the fixture to pass a properly typed IHistoryTx object directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/new/hathorwallet.test.ts` around lines 397 - 401, The test uses an
unsafe double-cast to supply websocket history as an array; replace the enqueue
payload with a properly shaped IHistoryTx object so onNewTx's
IHistoryTxSchema.safeParse(wsData.history) receives the expected single-object
shape. Locate the test code setting hWallet.wsTxQueue.enqueue and change the
history property to a valid IHistoryTx instance (matching the IHistoryTx
type/fields used by IHistoryTxSchema) instead of `[1] as unknown as IHistoryTx`,
ensuring WalletWebSocketData.history conforms to IHistoryTx.
There was a problem hiding this comment.
I'm aware of this. Please keep raising this point in other PRs, but the context of this specific PR is only to change the typings, and not the code or the tests.
There was a problem hiding this comment.
@tuliomir, understood! I appreciate the clarification on the scope. Since this PR focuses exclusively on adding type definitions without altering test or code behavior, I'll note this issue for future PRs where test fixture corrections would be in scope.
I'll continue to flag this type mismatch when reviewing related changes. Thank you for keeping me informed about the boundaries of this effort! 👍
| this.storage.setTxSignatureMethod(method); | ||
| if (method) { | ||
| this.storage.setTxSignatureMethod(method); | ||
| } |
There was a problem hiding this comment.
Why do we only write to storage if the method is present?
This makes it so we cannot "clean" the tx sign method.
I know this is a very unlikely case, but we should at least make it possible, right?
There was a problem hiding this comment.
Good catch. The problem was that the storage method itself was incorrectly typed. I had to change two more files to fix it, and then the logic I broke was fixed as well. Done on 985d170
github-project-automation
bot
moved this from In Progress (Done)
to In Review (WIP)
in Hathor Network
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
3 participants
Acceptance Criteria
Security Checklist
Summary by CodeRabbit
Refactor
Tests
Chores