-
Notifications
You must be signed in to change notification settings - Fork 15
docs: Fullnode Typings [11] - Connection and Websockets #1013
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
Changes from all commits
faf01eb
144968f
863bbf8
0648fb0
4b97cc2
75f9196
ebd8065
895a41f
d30c16c
5279062
4fbf25f
2b0aecf
88bc20e
90763dc
985d170
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,8 @@ import { | |
| } from '../../src/constants'; | ||
| import { MemoryStore, Storage } from '../../src/storage'; | ||
| import Queue from '../../src/models/queue'; | ||
| import { WalletType } from '../../src/types'; | ||
| import { IHistoryTx, WalletType } from '../../src/types'; | ||
| import { WalletWebSocketData } from '../../src/new/types'; | ||
| import txApi from '../../src/api/txApi'; | ||
| import * as addressUtils from '../../src/utils/address'; | ||
| import walletUtils from '../../src/utils/wallet'; | ||
|
|
@@ -370,13 +371,17 @@ test('processTxQueue', async () => { | |
| }; | ||
|
|
||
| // wsTxQueue is not part of the prototype so it won't be faked on FakeHathorWallet | ||
| hWallet.wsTxQueue = new Queue(); | ||
| hWallet.wsTxQueue.enqueue(1); | ||
| hWallet.wsTxQueue.enqueue(2); | ||
| hWallet.wsTxQueue.enqueue(3); | ||
| hWallet.wsTxQueue = new Queue<WalletWebSocketData>(); | ||
| hWallet.wsTxQueue.enqueue({ type: 'fakeType' }); | ||
| hWallet.wsTxQueue.enqueue({ type: 'fakeType' }); | ||
| hWallet.wsTxQueue.enqueue({ type: 'fakeType' }); | ||
|
|
||
| await hWallet.processTxQueue(); | ||
| expect(processedTxs).toStrictEqual([1, 2, 3]); | ||
| expect(processedTxs).toStrictEqual([ | ||
| { type: 'fakeType' }, | ||
| { type: 'fakeType' }, | ||
| { type: 'fakeType' }, | ||
| ]); | ||
| }); | ||
|
|
||
| test('handleWebsocketMsg', async () => { | ||
|
|
@@ -389,8 +394,11 @@ test('handleWebsocketMsg', async () => { | |
| }); | ||
|
|
||
| // wsTxQueue is not part of the prototype so it won't be faked on FakeHathorWallet | ||
| hWallet.wsTxQueue = new Queue(); | ||
| hWallet.wsTxQueue.enqueue({ type: 'wallet:address_history', history: [1] }); | ||
| hWallet.wsTxQueue = new Queue<WalletWebSocketData>(); | ||
| hWallet.wsTxQueue.enqueue({ | ||
| type: 'wallet:address_history', | ||
| history: [1] as unknown as IHistoryTx, | ||
| }); | ||
|
Comment on lines
+397
to
+401
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll continue to flag this type mismatch when reviewing related changes. Thank you for keeping me informed about the boundaries of this effort! 👍 |
||
| hWallet.newTxPromise = Promise.resolve(); | ||
|
|
||
| hWallet.state = HathorWallet.PROCESSING; | ||
|
|
@@ -559,6 +567,7 @@ test('getAddressPrivKey', async () => { | |
| getCurrentServer: jest.fn().mockReturnValue('https://fullnode'), | ||
| on: jest.fn(), | ||
| start: jest.fn(), | ||
| getCurrentNetwork: jest.fn().mockReturnValue('testnet'), | ||
| }; | ||
|
|
||
| jest.spyOn(versionApi, 'getVersion').mockImplementation(resolve => { | ||
|
|
@@ -596,6 +605,7 @@ test('signMessageWithAddress', async () => { | |
| getCurrentServer: jest.fn().mockReturnValue('https://fullnode'), | ||
| on: jest.fn(), | ||
| start: jest.fn(), | ||
| getCurrentNetwork: jest.fn().mockReturnValue('testnet'), | ||
| }; | ||
|
|
||
| jest.spyOn(versionApi, 'getVersion').mockImplementation(resolve => { | ||
|
|
@@ -725,6 +735,7 @@ test('start', async () => { | |
| getCurrentServer: jest.fn().mockReturnValue('https://fullnode'), | ||
| on: jest.fn(), | ||
| start: jest.fn(), | ||
| getCurrentNetwork: jest.fn().mockReturnValue('testnet'), | ||
| }; | ||
|
|
||
| jest.spyOn(versionApi, 'getVersion').mockImplementation(resolve => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,10 +25,10 @@ module.exports = { | |
| }, | ||
| // We need a high coverage for the HathorWallet class | ||
| './src/new/wallet.ts': { | ||
| statements: 92, | ||
| branches: 85, | ||
| statements: 90, | ||
| branches: 83, | ||
| functions: 90, | ||
| lines: 92, | ||
| lines: 90, | ||
|
Comment on lines
-28
to
+31
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }, | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,13 @@ abstract class Connection extends EventEmitter { | |
| this.emit('state', state); | ||
| } | ||
|
|
||
| /** | ||
| * Get current connection state | ||
| */ | ||
| getState(): ConnectionState { | ||
| return this.state; | ||
| } | ||
|
Comment on lines
+118
to
+120
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All code used to access |
||
|
|
||
| /** | ||
| * Connect to the server and start emitting events. | ||
| * */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,7 @@ export function isDataOutput(output: ISendOutput): output is ISendDataOutput { | |
| } | ||
|
|
||
| export interface ISendTokenOutput { | ||
| type: OutputType.P2PKH | OutputType.P2SH; | ||
| type: OutputType.P2PKH | OutputType.P2SH; // XXX: This type is ignored in the only place it is used | ||
| address: string; | ||
| value: OutputValueType; | ||
| token: string; | ||
|
|
@@ -506,7 +506,7 @@ export default class SendTransaction extends EventEmitter { | |
| // This just returns if the transaction is not a CREATE_TOKEN_TX | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type expected from the fullnode is But I think we can change it to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| })(this.wallet, this.storage, this.transaction); | ||
| } | ||
| this.emit('send-tx-success', this.transaction); | ||
|
|
||
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.
since you already added the getState in the connectionFile. What about use it here?
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.
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.