-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implement WebSocket to Stream Silent Blocks #46
base: main
Are you sure you want to change the base?
Implement WebSocket to Stream Silent Blocks #46
Conversation
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.
A few more thoughts:
- I see that you are injecting
SilentBlocksGateway
intoSilentBlocksService
. That is not the standard pattern. Gateways are like Controllers. You don't inject controllers into a service, you inject service into a controller. Therefore, please remove the event handling logic fromSilentBlocksService
and move it to inside ofSilentBlocksGateway
. - I also think we should use
socket.io
and leverage the concept of rooms. Once a client connects to use, we can subscribe them to aSILENT_BLOCK
room. Then we just need to publish silent block to that room and let socket.io handle it from there. This will eliminate the need offorEach
loop inbroadcastSilentBlock
Okay, changes noted |
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.
I don't think you made all the changes I requested. Please do so before I review it again.
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.
Please do the following:
- fix linting issues
- fix broken unit tests
- squash all commits into just one commit
config/dev.config.yaml
Outdated
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.
revert please
package-lock.json
Outdated
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.
the lock file is not up to date with package.json
diff --git a/package-lock.json b/package-lock.json
index c0b2e37..05d3cd2 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -17,6 +17,7 @@
"@nestjs/passport": "^10.0.0",
"@nestjs/platform-express": "^10.3.7",
"@nestjs/platform-socket.io": "^10.3.7",
+ "@nestjs/platform-ws": "^10.4.4",
"@nestjs/schedule": "^3.0.0",
"@nestjs/swagger": "^7.3.1",
"@nestjs/typeorm": "^10.0.2",
@@ -32,7 +33,6 @@
},
"devDependencies": {
"@nestjs/cli": "^10.4.5",
- "@nestjs/platform-ws": "^10.4.4",
"@nestjs/schematics": "^9.0.0",
"@nestjs/testing": "^10.3.7",
"@types/express": "^4.17.13",
@@ -2044,7 +2044,6 @@
"version": "10.4.4",
"resolved": "https://registry.npmjs.org/@nestjs/platform-ws/-/platform-ws-10.4.4.tgz",
"integrity": "sha512-6E476YvfO14uQUT6FzWFpGnwp58fpGq2aeGxHFdRMMptOQ5suKqD+3LsZgPiGF1elgVhQcI5uqM15qL3yH+OqQ==",
- "dev": true,
"dependencies": {
"tslib": "2.7.0",
"ws": "8.18.0"
@@ -2062,14 +2061,12 @@
"node_modules/@nestjs/platform-ws/node_modules/tslib": {
"version": "2.7.0",
"resolved": "https://registry.npmjs.org/tslib/-/tslib-2.7.0.tgz",
- "integrity": "sha512-gLXCKdN1/j47AiHiOkJN69hJmcbGTHI0ImLmbYLHykhgeN0jVGola9yVjFgzCUklsZQMW55o+dW7IXv3RCXDzA==",
- "dev": true
+ "integrity": "sha512-gLXCKdN1/j47AiHiOkJN69hJmcbGTHI0ImLmbYLHykhgeN0jVGola9yVjFgzCUklsZQMW55o+dW7IXv3RCXDzA=="
},
"node_modules/@nestjs/platform-ws/node_modules/ws": {
"version": "8.18.0",
"resolved": "https://registry.npmjs.org/ws/-/ws-8.18.0.tgz",
"integrity": "sha512-8VbfWfHLbbwu3+N6OKsOMpBdT4kXPDDB9cJk2bJ6mh9ucxdlnNvH1e+roYkKmN9Nxw2yjz7VzeO9oOz2zJ04Pw==",
- "dev": true,
"engines": {
"node": ">=10.0.0"
},
@@ -44,6 +46,7 @@ export class BitcoinCoreProvider | |||
private readonly configService: ConfigService, | |||
indexerService: IndexerService, | |||
operationStateService: OperationStateService, | |||
private eventEmitter: EventEmitter2, |
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.
private eventEmitter: EventEmitter2, | |
private readonly eventEmitter: EventEmitter2, |
@@ -29,6 +30,8 @@ export class EsploraProvider | |||
private readonly configService: ConfigService, | |||
indexerService: IndexerService, | |||
operationStateService: OperationStateService, | |||
private eventEmitter: EventEmitter2, |
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.
private eventEmitter: EventEmitter2, | |
private readonly eventEmitter: EventEmitter2, |
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.
We need to emit INDEXED_BLOCK_EVENT
event in this provider as well
@OnEvent(INDEXED_BLOCK_EVENT) | ||
async handleBlockIndexedEvent(blockHeight: number) { | ||
this.logger.debug(`New block indexed: ${blockHeight}`); | ||
await delay(1000); |
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.
Why are we adding a delay here?
this.server.clients.forEach((client: WebSocket) => { | ||
if (client.readyState === WebSocket.OPEN) { | ||
client.send(silentBlock.toString('hex')); // Send silent block as hex string | ||
} | ||
}); |
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.
- because everything inside
forEach
is a synchronous operation, we should use a simplefor..of
loop.forEach
is very slow. - I see that you are sending the block as a hex string. This breaks the pattern we use for REST endpoints. We send the block as
Buffer
in REST endpoints. I think we should stick to usingBuffer
here as well for consistency.
Please Rebase From main |
Okay |
3f0b282
to
4caad8e
Compare
fixed event emitter issue fix: Add a delay function for silent block events fix: silent block broadcasting Fix: Use a more explicit event name for indexing blocks feat: implement silent block broadcasting fix: Make event emitter protected feat: implement silent block broadcasting fix: Add event emitter and gateway dependencies to tests chore: added makerfile to make local e2e test a breeze added makerfile to make local e2e test a breeze: working improved maker file structure with output logs for debug purpose chore: update vulnerable dependancies Signed-off-by: Anmol Sharma <[email protected]> feat: implement silent block broadcasting fixed event emitter issue fix: Add a delay function for silent block events fix: silent block broadcasting Fix: Use a more explicit event name for indexing blocks feat: implement silent block broadcasting fix: Make event emitter protected feat: implement silent block broadcasting fix: Add event emitter and gateway dependencies to tests
1af0ec3
to
0bb8e57
Compare
Objective(s):
Implement a WebSocket connection to stream silent blocks to the client whenever a new block is found and indexed. This will eliminate the need for the client to keep polling for new blocks.
Changes:
ws
library.base-block-data-provider.abstract.ts
that emits an event whenever a new block is indexed.SilentBlocksService
and trigger the silent block broadcasting logic when the event is fired.Scope of Change:
This change is semi-critical. While it doesn't directly affect core functionality, it significantly enhances the user experience and system efficiency by providing real-time updates.
solves #45